-
Notifications
You must be signed in to change notification settings - Fork 271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modify proxy to pass full certificate before login #896
Conversation
082b33d
to
b66d2f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @sarahcaseybot)
proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java, line 154 at r1 (raw file):
Base64.getEncoder().encodeToString(sslClientCertificate.getEncoded())); } catch (CertificateEncodingException e) { throw new RuntimeException("Cannot encode client certificate.", e);
Nit: No periods at end of exception messages. (This is how Java's standard library does it; I don't make the rules, I promise!)
proxy/src/test/java/google/registry/proxy/TestUtils.java, line 123 at r1 (raw file):
.headers() .set( "X-SSL-Full-Certificate", Base64.getEncoder().encodeToString(certificate.getEncoded()));
This is duplicating the logic under test, and thus isn't actually testing that the encoding is working properly.
Can you have the test below use an actual hard-coded Base64-encoded string? That way if the encoder behavior changes for some reason, we'll actually get a test failure, rather than the test continuing to pass but with different behavior.
b66d2f4
to
5a7efa1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @CydeWeys)
proxy/src/test/java/google/registry/proxy/TestUtils.java, line 123 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
This is duplicating the logic under test, and thus isn't actually testing that the encoding is working properly.
Can you have the test below use an actual hard-coded Base64-encoded string? That way if the encoder behavior changes for some reason, we'll actually get a test failure, rather than the test continuing to pass but with different behavior.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @CydeWeys)
This change is