Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[nexus] Add HTTPS support, plumbing x509 certificates #1500
[nexus] Add HTTPS support, plumbing x509 certificates #1500
Changes from 9 commits
1f843f1
920ad13
494a6f1
3ede000
0104f49
1cf097a
b58e10a
2926fe0
edea7b9
8137419
eb1ea85
95c10a0
6502c44
0f0afbb
31a93ea
cf4864b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is there an analogous change for
how-to-run-simulated
? In general, the test suite matches whathow-to-run-simulated
does so if you've updated that then things should work.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.
There is not, because the implementation within Nexus checks for the existence of the certificate files before deciding whether or not to launch the HTTPS server.
For the simulated server, they won't exist, so only the HTTP server will be launched.
I definitely think this would be required before moving to "HTTPS only", but considered this PR an intermediate step.
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.
Mentioned in the other comment, but that doesn't seem like the right behavior. It seems like in production, if they don't exist, that should be a fatal error. And in development, if they do exist, we should start the HTTP server. If this were part of configuration, then whoever's running Nexus can express their intent. (I'm not saying we have to do all that in this PR, but if we decide that is what we want, then I think this behavior is not really an intermediate step.)
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.
TODO(me): I wonder if I can fix #1398 and make this easier at the same time?
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.
See: #1528. We're gonna need to grab / insert certs from somewhere.
I've updating the packaging hints to make this more obvious if an error is encountered.
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.
This seems unlikely to work on development machines.
Why not make this part of
dropshot_external
, with/var/nexus/certs/cert.pem
the value in the SMF config file and something like "out/certs" the value in nexus/examples/config.toml?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.
FWIW, this file is only used
if cert_file.exists() && key_file.exists()
.I was kinda opposed to using the config-file-supplied certificate, because longer-term, it seemed like this value would be:
So although we are currently plumbing the certificates through the package system, I don't expect that to stay the case for very long.
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.
What happens if the path is missing on a production system? It seems like that should produce a fatal error.
I can see where you're coming from. At the same time, it seems like an essential developer or demo tool to be able to start Nexus with a specific certificate?
Is that a good idea for the production config? I'm not sure. It might depend on the expected behavior in the longer term. Is it that Nexus reaches out to CockroachDB/Vault to get the certificate, then loads that into memory, and then starts the HTTPS server with it? If that's true, then what do we do if Vault is down? I could see: (1) not coming up at all until we can get the cert, (2) storing the last-known-good certificate to a file and trying to use that until we can get the cert, (3) generating a self-signed certificate and using that until we're able to access the real one. These all have different tradeoffs. I've been assuming Nexus could come up (albeit not very usefully) even if everything else in the world was down so that we could at least ask it why it wasn't able to come up. Anyway, if we go with (2) or (3), then it seems like accepting a file path in the config file would be a reasonable stepping stone.
(Along these lines, I guess hardcoded, deployment-specific absolute paths in source files seem like a code smell to me, even if we gracefully handle them being missing.)