Closed jwedel closed 4 years ago
I like those suggestions. The introduction of a value object for a TAN is something I thought about suggesting as well. Would bind the verification logic into a value and make the using code more expressive and safe.
Although I am more of a hypermedia guy and thus not a big fan of hard coding URIs in clients, I think Feign would be fine here, although I think that – unless also used in other places – the benefit of using that over RestTemplate
/WebClient
is not that significant.
What I do think needs improvement is the setup of whatever client API is ended up being used. Remote calls definitely need a timeout as otherwise the system is stuck in case the downstream service is not answering at all for whatever reasons.
Also, from a security perspective this implementation shall be changed. When building the json string this way, a malicious tan can inject arbitrary fields to the json object. Even if the tan is previously checked for the correct syntax in another method, this implementation is error-prone. The syntax check for the tan can be called directly before creating the json string or the tan can be filtered with an whitelist approach.
I assume the previously mentioned changes will also tackle the injection issue.
I haven't really looked at the verification server but calling as POST to verify a TAN seems to me a bit unRESTy to be honest. But this is probably worth another issue...
I haven't really looked at the verification server but calling as POST to verify a TAN seems to me a bit unRESTy to be honest. But this is probably worth another issue...
It actually explained in the documentation:
The HTTP method POST is used instead of GET for added security, so data (e.g. the registration token or the TAN) can be transferred in the body.
However, I would still doubt that it's necessary.
Correct me if I'm wrong, but when doing backend-to-backend http call, the whole request - both URL params and and body - are within the same TCP packet. So as long as either:
This is probably only important when doing a frontend-to-backend call which is not the case here.
We have discussed and would like to proceed with the FeignClient, however, with the approach of keeping the java class entity on cwa-server, simply having one attribute 'tan'
I haven't really looked at the verification server but calling as POST to verify a TAN seems to me a bit unRESTy to be honest. But this is probably worth another issue...
It actually explained in the documentation:
The HTTP method POST is used instead of GET for added security, so data (e.g. the registration token or the TAN) can be transferred in the body.
However, I would still doubt that it's necessary.
Correct me if I'm wrong, but when doing backend-to-backend http call, the whole request - both URL params and and body - are within the same TCP packet. So as long as either:
- Both backends are within a secure environment or
- The use TLS I don't think moving the tan to the body helps to improve security in any way.
This is probably only important when doing a frontend-to-backend call which is not the case here.
The main reason to go with POST here (which transports the secret in the http body) is due to the sensitive nature of the TAN. We don't want the TAN to come up in any of our infrastructure / http access logs by chance.
This does not only apply to TANs but to all other sensitive data we are processing.
I'm working on a PR.
Current Implementation
The current tan verification look like this: https://github.com/corona-warn-app/cwa-server/blob/3b98beb268bac511c00f8e13bcbb4978a8cdb4fd/services/submission/src/main/java/app/coronawarn/server/services/submission/verification/TanVerifier.java#L102-L115
There are a couple of issues I would see here:
Suggested Enhancement
I would suggest two things
@RestController
and supports the same annotations. It will create the client implementation from the given interface at runtime.The client would look like this:
The usage would look like this (I did not actually implemented that!):
Expected Benefits