-
Notifications
You must be signed in to change notification settings - Fork 485
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
Introduce EJBCA UpstreamAuthority plugin for SPIRE Server #5378
Introduce EJBCA UpstreamAuthority plugin for SPIRE Server #5378
Conversation
Signed-off-by: Hayden Roszell <hroszell@gmail.com>
refactor EJBCA config to only support certs from file Signed-off-by: Hayden Roszell <hroszell@gmail.com>
Signed-off-by: Hayden Roszell <hroszell@gmail.com>
…BCA docs Signed-off-by: Hayden Roszell <hroszell@gmail.com>
Signed-off-by: Hayden Roszell <hroszell@gmail.com>
Signed-off-by: Hayden Roszell <hroszell@gmail.com>
…r to use non-OAuth server config Signed-off-by: Hayden Roszell <hroszell@gmail.com>
Signed-off-by: Hayden Roszell <hroszell@gmail.com>
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.
Thank you @m8rmclaren for addressing the previous comments. I've added some more comments/suggestions.
config.ClientKeyPath = p.hooks.getEnv("EJBCA_CLIENT_CERT_KEY_PATH") | ||
} | ||
|
||
if config.ClientCertPath == "" { |
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 check can be done earlier, before configuring ClientKeyPath.
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.
Hi @amartinezfayo!
I wrote it in this order to set the corresponding variable from the environment if it wasn't set in the conf file, and if neither is defined, return an error.
If there is a more elegant way to set ClientCertPath
and ClientCertKeyPath
from the environment if not present in the conf file, I'm open to suggestions.
For now, I just wrote more descriptive comments.
} | ||
enrollConfig := ejbcaclient.EnrollCertificateRestRequest{} | ||
enrollConfig.SetUsername(endEntityName) | ||
enrollConfig.SetPassword(password) |
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.
How is this password conveyed with the server?
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.
Hi @amartinezfayo!
This password (enrollment code) is conveyed to EJBCA in the POST /v1/certificate/pkcs10enroll
request and has no direct effect concerning the EJBCA plugin.
Signed-off-by: Hayden Roszell <hroszell@gmail.com>
Signed-off-by: Hayden Roszell <hroszell@gmail.com>
…hority Signed-off-by: Hayden Roszell <hroszell@gmail.com>
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.
Thank you @m8rmclaren for your patience and addressing the comments.
He have introduced some recent changes (#5340) that requires replacing the use of x509certificate.ToPluginProtos with x509certificate.ToPluginFromCertificates.
I think that we should be good to go after updating that.
Signed-off-by: Hayden Roszell <hroszell@gmail.com>
Hi @amartinezfayo! Thank you for pointing this out; I've updated all usages of |
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.
Thank you @m8rmclaren for this contribution!
Pull Request check list
Affected functionality
This PR introduces EJBCA as a Server UpstreamAuthority plugin. This plugin uses a connected EJBCA to issue intermediate certificates for the SPIRE server.
Description of change
Which issue this PR fixes