Skip to content
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

Merged
merged 15 commits into from
Sep 5, 2024

Conversation

m8rmclaren
Copy link
Contributor

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

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.

Per #4163, the EJBCA UpstreamAuthority plugin is compatible with EJBCA Community.

Description of change

  • Adds EJBCA as a built-in UpstreamAuthority plugin.
  • Adds the EJBCA Go Client SDK as a required dependency for communicating with the EJBCA REST API.

Original PR was #5201 which was closed to rename the base branch from main to a more descriptive name.

Which issue this PR fixes

Copy link
Member

@amartinezfayo amartinezfayo left a 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.

pkg/server/plugin/upstreamauthority/ejbca/ejbca.go Outdated Show resolved Hide resolved
doc/plugin_server_upstreamauthority_ejbca.md Outdated Show resolved Hide resolved
config.ClientKeyPath = p.hooks.getEnv("EJBCA_CLIENT_CERT_KEY_PATH")
}

if config.ClientCertPath == "" {
Copy link
Member

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.

Copy link
Contributor Author

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.

pkg/server/plugin/upstreamauthority/ejbca/ejbca.go Outdated Show resolved Hide resolved
pkg/server/plugin/upstreamauthority/ejbca/ejbca.go Outdated Show resolved Hide resolved
pkg/server/plugin/upstreamauthority/ejbca/ejbca.go Outdated Show resolved Hide resolved
pkg/server/plugin/upstreamauthority/ejbca/ejbca.go Outdated Show resolved Hide resolved
pkg/server/plugin/upstreamauthority/ejbca/ejbca.go Outdated Show resolved Hide resolved
doc/plugin_server_upstreamauthority_ejbca.md Show resolved Hide resolved
}
enrollConfig := ejbcaclient.EnrollCertificateRestRequest{}
enrollConfig.SetUsername(endEntityName)
enrollConfig.SetPassword(password)
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

@amartinezfayo amartinezfayo left a 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.

pkg/server/plugin/upstreamauthority/ejbca/ejbca.go Outdated Show resolved Hide resolved
pkg/server/plugin/upstreamauthority/ejbca/ejbca.go Outdated Show resolved Hide resolved
@m8rmclaren
Copy link
Contributor Author

Hi @amartinezfayo!

Thank you for pointing this out; I've updated all usages of ToPluginProtos to ToPluginFromCertificates. The tests are now passing in my GitHub Actions.

Copy link
Member

@amartinezfayo amartinezfayo left a 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!

@amartinezfayo amartinezfayo merged commit 7118533 into spiffe:main Sep 5, 2024
34 checks passed
@amartinezfayo amartinezfayo added this to the 1.11.0 milestone Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants