Admin post certificate #120
Conversation
lib io/ioutil Signed-off-by: Paul Gaiduk <[email protected]>
* Added an endpoint at /admin/certs that will accept incoming POST requests to set Adam's signing certificate * Changed configs to reference pathes to certificates and keys instead of copying * Added utils to write files atomically Signed-off-by: Paul Gaiduk <[email protected]>
europaul
force-pushed
the
admin-post-certificate
branch
from
9adb667
to
4f3b5f3
Compare
|
@deitch Please review when you have some extra time. Paul has prepared a test which depends on this PR: lf-edge/eden#927 |
| return | ||
| } | ||
| // set the signing cert path | ||
| *h.signingCertPath = certPath |
There was a problem hiding this comment.
As I understand we try to override the variable which come from cli here. I can see that we potentially may create new file few lines above, how we will use it in case of adam's container restart?
I still cannot understand, is the approach you define here better than just manual file override. Looks like restart of adam is not required to re-read the certificate.
There was a problem hiding this comment.
We only create a different file in case that CLI option was set to an empty string, otherwise we rewrite the certificate file. I think it's pretty unlikely that somebody unsets the CLI option explicetly, if it's not set we use the default value. So if Adam is restarted with the same CLI params it will pick up the new certificate. Only if it's restarted with --signing-cert="" it will not pick up any certificate and we'll need to set it again with the POST request.
I didn't understand how you are proposing to replace the certificate file from Eden without an API call? I thought they are only supposed to communicate through the API and we cannot assume that Adam and Eden are running on the same system, so that we can just replace the file locally.
There was a problem hiding this comment.
it's pretty unlikely that somebody unsets the CLI option explicitly
well, sounds reasonable, it will look like a problem of the caller.
I didn't understand how you are proposing to replace the certificate file from Eden without an API call? I thought they are only supposed to communicate through the API and we cannot assume that Adam and Eden are running on the same system, so that we can just replace the file locally.
right now we prepare the options to start Adam container in Eden. And we generate certs on Eden side. But looks like we use environment variables to set signing cert here. So we cannot change the certificate without Adam's restart (StopAdam + StartAdam calls) in current logic and you approach looks valid. But can you please check that we will have no problems without file replacement after Adam restart (I mean, will fill correct SIGNING_CERT env), e.g. eden adam stop + eden adam start?
There was a problem hiding this comment.
upon the start of adam server, we always take the certificate from the env - so the one that's provided by the caller. So now it looks like it's eden's responsibility to provide the new certificate / replace the certificate file etc.
There was a problem hiding this comment.
btw, why is the adam stop command doing the exact opposite of what --adam-rm requests? and all the other stop commands as well?
There was a problem hiding this comment.
yes, but using POST is cleaner because
- we can use it in a scenario when the caller (like Eden) and Adam are not colocated - through the network
- if the certificate resides in the database path (like done by default), we shouldn't probably just manipulate files there, circumventing Adam's interfaces
There was a problem hiding this comment.
if the certificate resides in the database path (like done by default), we shouldn't probably just manipulate files there, circumventing Adam's interfaces
100%. This usage of --cert-path is just wrong. Either there should be no --cert-path option, or if there is one, it is outside the database and is a read-only space that Adam uses to get the content, never to write it.
we can use it in a scenario when the caller (like Eden) and Adam are not colocated - through the network
Don't you find it strange to change those certs over the network? No other server software of which I am aware does that.
If you do, you are saying, "cert is a database artifact". In that case:
--cert-pathmust be removed; it is fixed in the database- cert must be stored in the actual database, based on the driver (file, redis, memory, etc.), but entirely inside the database; Adam has no knowledge of what it is or where it goes, just passed it to the database driver or reads it from the database driver
- the env var must be resolved. Does it override what is in the database? Or seed it? Override seems to make sense, unless we rename it something that says very clearly, "this is the seed only, but if something is in the database I will use that". Which still is a bit odd behaviour.
Doesn't GET /certs return multiple certs? What would your proposed POST do?
There was a problem hiding this comment.
I was talking about the POST implemented as part for this PR, just for the signing certificate
There was a problem hiding this comment.
What does GET /certs get? Is that just the signing certificate as well?
There was a problem hiding this comment.
I added POST /admin/certs. There is no GET /admin/certs
This commit refactors the code related to creating certificates and keys. It now checks if the files already exist before creating new ones. This way Adam doesn't overwrite those in case of a restart. Signed-off-by: Paul Gaiduk <[email protected]>
|
After all of this, I think I finally understand why you did what you did @europaul . You viewed the env var as the equivalent of "load cert material into the database", and So the POST was a simple way of being equivalent to the env var. We had viewed the CLI flag as definitive, like if you passed it to nginx or apache, i.e. a way to tell Adam a location to source the certificate. |
|
as per the discussion above, we decided to change the approach and go with #123 instead. |
to set Adam's signing certificate
of copying
Closes #118