Closed eudyptula closed 6 years ago
I will check these, and write a detailed answer tomorrow.
Ok, was stopping for today anyways ;)
1) Yes, I don't see any problem with that. 2) I think if isSecure is true but the creation of securityContext fails, the client should just exit with an exception. 3) Right now it is that way on purpose. If I understand its Javadocs correctly, the secure client can terminate the connection (in the handshake phase), if the hostname and the servers first response about its identity are not in sync (according to some default rules). 4) I dont think it's important to do so. My main motivation behind using the builder pattern with the SRF was to simplify building one for the client, since it has 5+ possible properties. OrchestrationForms are only created by the Orchestrator, and it uses the 3 parameter constructor + setters only because the code logic dictates it that way. A 3 parameter constructor does not warrant a builder pattern in my opinion. 5) BasicConsumer should certainly stay. The proof of concept cert_request module is kinda redundant now, since I moved its logic into the client-common module too. I did this module as a test, before creating the CertificateBootstrapper class. I think we can delete it.
Please check the arrowhead-dev slack channel, I asked a question there.
In CertificateAuthorityClient::bootstrap() Could we skip the needAuth parameter and just always download the AUTH pubkey?
In RestClient the securityContext is (sometimes) the deciding factor on whether we are in secure or insecure mode. I wonder if this would confuse the users, so they'd think they are using secure mode but really aren't. For example: If a Consumer calls
RestClient.create(httpsUri, null)
the https in the uri will be overridden by http and the result is insecure mode. Any ideas on how to prevent these misunderstandings?In SecurityUtils I noticed a HostnameVerifier just returning true in all cases (I might have moved it there from another class). Is this by design or something that has to be fixed in the future?
I noticed the ServiceRequestForm uses the builder pattern - which helped me call the validate method on every build. Should we change the OrchestrationForm to the same pattern for consistency?
Are we keeping the BasicConsumer and the cert_request? I haven't touched these at all.