On segunda-feira, 18 de julho de 2016 14:07:58 PDT Abhishek Sharma wrote: > > The second issue is the mapping at all. I would argue that the client of > > the proxy should be able to get the data in any format that they want. > > For example, many weather services provide their data in XML, not JSON. > > Limiting the proxy support for JSON only is short-sighted. Client should > > be able to download small binary blobs of data, images, XML, text, JSON > > (without parsing!), CBOR, etc. Please pass the data through, unchanged. > > [Abhi] The proxy solution is for constrained OCF clients that can't afford > to have parallel http implementation but still want to make use of > INFORMATION available on cloud. It is highly unlikely that a constrained > client which chose not to implement "http" would want a full fledged XML or > HTML parser.
XML parsers are not very difficult to write, even for small, constrained implementations. Yes, they mean extra code for the constrained device, but I would say it's far more likely to produce a usable functionality than constraining the remote service to JSON that can be converted to the current use of CBOR that we do (for example, arrays limited to 3 levels of depth). I'm sorry, any solution that doesn't include support for other content types is flawed and shouldn't be accepted. > Also allowing these different types of data will violate > current IoTivity communication which only mandates CBOR for communication > between clients and server's. Indeed. But that can be solved in two ways: 1) move the fuctionality to another CoAP port, which does proxying only, no OCF protocol. 2) send the content of the reply as a Byte Text payload. That is, one single "property". I think you should analyse whether either makes sense and figure out the one that violates RESTful requirements the least. > > Just look at how your current implementation does not support HTTPS > > properly (or at all, since I couldn't find it). HTTPS support is > > MANDATORY. Do you really want to write the code for it? TinyDTLS is not > > going to cut for it. Don't forget checking the SSL certificates based on > > the OS's store, checking CN, etc. > > [Abhi] No we will not be writing code for "https" and will definitely be > using a third party library. Its just not part of plan for the first > version of Proxy and will be added in subsequent iterations. Not acceptable. HTTPS support should be there in the first version. We must not release a version of this proxy without HTTPS, as that would make an incentive for people to depend on non-HTTPS functionality. > > I pointed this out in the JIRA and you responded that size was a concern. > > No, it's not. First of all, IoTivity does not run on constrained devices. > > Any device where it can run will be able to accommodate libcurl. Second, > > the whole point of a CoAP-HTTP Proxy is that a *bigger* device can > > provide a service for a smaller one. > > [Abhi] Bigger device doesn't mean that we shall essentially use bulky 3rd > party libraries. The current parser implementation is good enough for > the HTTP parser requirements we had. For https support we might require an > upgrade though. Apart from the binary, runtime memory usage etc, libcurl > also has some dependency and license issues > (https://curl.haxx.se/legal/licmix.html). What to and what not to include > will require quite some licensing expertise. Then please do that homework and figure out what third party library to use. The choice must be of one that is well-known and established, routinely receives updates. As I said, we must not release a version without HTTP support, so please plan this. I find that your implementation, using a non-established parser on top of a self-written TCP stack not intended for use with the cloud is extremely fragile and will produce more maintenance burden for us. > > A) Location in the source code > > This is a service. It should not be in "resource/". > > [Abhi] There is a reason why we can't move it to service. RI layer callbacks > work on the basis of resource uri. For ex: when multiple resources are > hosted, RI will maintain mapping between hosted resource and respective > callbacks. But as per CoAP spec, any CoAP request to proxy (having > "Proxy-Uri" option) MUST NOT contain other parameters like URI-Host etc. ( > https://tools.ietf.org/html/rfc7252#section-5.10.2 ) Sorry, that's an excuse. Please move it to resource or out-of-tree and modify the IoTivity API so that your requirements can be met. Second, you're not writing a CoAP proxy. You've argued above as a justification for using CBOR only that you're writing an OCF resource that takes OCF payload and sends an OCF payload as a reply. If your argument for CBOR holds, then you do not need to follow any of the CoAP requirements for proxying, as your service is not a CoAP proxy. Or, alternatively, make it a real CoAP proxy, on a separate port from the OCF port. If you do that, you'll be able to manipulate the headers as you see fit, and you will be able to accept the headers the way the CoAP spec says you should. You'll probably need to interface with libcoap directly, not using the CA and RI layer, though. > > Within two hours of reviewing your code, I found numerous memory leaks, > > buffer overruns and possible out-of-bounds access, not to mention code > > that has clearly never been tested. It has use of untrusted data in > > memory allocation, which is a clear verctor for remote attacks. > > [Abhi] Static code analysis was completed and a patch was uploaded today. In > next two days we will also complete thorough review and test team will > verify system test cases. Please provide any inputs that you deem fit or > your concerns on gerrit and it will be taken care of. It's too early for that. I'm making arguments for a complete refactor and redesign of your functionality, which invalidates your codebase almost completely. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center
