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.

Sign up for GitHub

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

Jump to bottom

Allow specifying a cert and a key manually for federation endpoint. #5163

Merged
merged 10 commits into from
Jun 18, 2024
Merged

Allow specifying a cert and a key manually for federation endpoint. #5163

merged 10 commits into from
Jun 18, 2024

Conversation

sorindumitru
Copy link
Contributor

Pull Request check list

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

Affected functionality
spire-server bundle endpoint

Description of change
Allow configuring the certificate used for the bundle endpoint server as files on disk instead of using ACME.

Which issue this PR fixes
fixes #2202

azdagron
azdagron reviewed May 23, 2024
View reviewed changes
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @sorindumitru. I've left some comments and there are others that were caught by the linter that should also be addressed.

support/oidc-discovery-provider/main.go Outdated
@@ -171,15 +172,19 @@ func newSource(log logrus.FieldLogger, config *Config) (JWKSSource, error) {
}

func newListenerWithServingCert(ctx context.Context, log logrus.FieldLogger, config *Config) (net.Listener, error) {
certManager, err := NewDiskCertManager(config.ServingCertFile, nil, log)
certManager, err := diskcertmanager.NewDiskCertManager(&diskcertmanager.DiskCertManagerConfig{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These names are stuttery. I'd suggest:

Suggested change
certManager, err := diskcertmanager.NewDiskCertManager(&diskcertmanager.DiskCertManagerConfig{
certManager, err := diskcertmanager.New(&diskcertmanager.Config{

pkg/server/endpoints/bundle/server_test.go Outdated
serverCert, serverKey := createServerCertificate(t)

serverCertPem := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: serverCert.Raw})
err := os.WriteFile(dir+"/server.crt", serverCertPem, 0755)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use filepath to create paths so they are platform independent.

Suggested change
err := os.WriteFile(dir+"/server.crt", serverCertPem, 0755)
err := os.WriteFile(filepath.Join(dir, server.crt), serverCertPem, 0600)

pkg/server/endpoints/bundle/server_test.go Outdated
dir := spiretest.TempDir(t)
serverCert, serverKey := createServerCertificate(t)

serverCertPem := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: serverCert.Raw})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we have helpers for this

Suggested change
serverCertPem := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: serverCert.Raw})
serverCertPem := pemutil.EncodeCertificate(serverCert.Raw)

pkg/server/endpoints/bundle/server_test.go Outdated
Comment on lines 191 to 193
serverKeyBytes, err := x509.MarshalPKCS8PrivateKey(serverKey)
require.NoError(t, err)
serverKeyPem := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: serverKeyBytes})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
serverKeyBytes, err := x509.MarshalPKCS8PrivateKey(serverKey)
require.NoError(t, err)
serverKeyPem := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: serverKeyBytes})
serverKeyBytes, err := pemutil.EncodePKCS8PrivateKey(serverKey)
require.NoError(t, err)

pkg/server/endpoints/bundle/server_test.go Outdated
serverKeyBytes, err := x509.MarshalPKCS8PrivateKey(serverKey)
require.NoError(t, err)
serverKeyPem := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: serverKeyBytes})
err = os.WriteFile(dir+"/server.key", serverKeyPem, 0700)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
err = os.WriteFile(dir+"/server.key", serverKeyPem, 0700)
err = os.WriteFile(filepath.Join(dir, "server.key"), serverKeyPem, 0600)

pkg/server/endpoints/config.go Outdated
} else if c.BundleEndpoint.DiskCertManager != nil {
serverAuth = c.BundleEndpoint.DiskCertManager
// Start watching for file changes
go c.BundleEndpoint.DiskCertManager.WatchFileChanges(ctx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This goroutine shouldn't be launched yet. Rather, maybeMakeBundleEndpointServer could return a "task" that that is set on the Endpoints struct, and then that task can be managed by Endpoints.ListenAndServe (see the other tasks conditionally started in ListenAndServe).

cmd/spire-server/cli/run/run.go Outdated
@@ -691,46 +699,53 @@ func NewServerConfig(c *Config, logOptions []log.Option, allowUnknownConfig bool
return sc, nil
}

func parseBundleEndpointConfigProfile(config *bundleEndpointConfig, dataDir string, log logrus.FieldLogger) (*bundle.ACMEConfig, error) {
func parseBundleEndpointConfigProfile(config *bundleEndpointConfig, federationConfig *server.FederationConfig, dataDir string, log logrus.FieldLogger) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a convention, I'd prefer parameters that are modifying by the function to be last. Also, maybe we should rename this function to imply that it is doing a little more than parsing:

Suggested change
func parseBundleEndpointConfigProfile(config *bundleEndpointConfig, federationConfig *server.FederationConfig, dataDir string, log logrus.FieldLogger) error {
func setBundleEndpointConfigProfile(config *bundleEndpointConfig, dataDir string, log logrus.FieldLogger, federationConfig *server.FederationConfig) error {

@sorindumitru
Move DiskCertManager to pkg/common
ef946a2 
Signed-off-by: Sorin Dumitru <sdumitru@bloomberg.net>
@sorindumitru
Rename TLSConfig to GetTLSConfig
fcfd337 
Makes it more compatible with the bundle endpoint authenticator

Signed-off-by: Sorin Dumitru <sdumitru@bloomberg.net>
@sorindumitru
spire-server: allow directly specifying a certificate and key for the…
1783071 
… bundle endpoint

Signed-off-by: Sorin Dumitru <sdumitru@bloomberg.net>
@sorindumitru
Update documentation
1d0b428 
Signed-off-by: Sorin Dumitru <sdumitru@bloomberg.net>
@sorindumitru
Start the disk manager watch files go-routine
6ac0bc3 
Signed-off-by: Sorin Dumitru <sdumitru@bloomberg.net>
@sorindumitru
address feedback
f55a682 
Signed-off-by: Sorin Dumitru <sdumitru@bloomberg.net>
@sorindumitru
Start file watcher as a task
c7eb48d 
Signed-off-by: Sorin Dumitru <sdumitru@bloomberg.net>
@sorindumitru
pacify linter
2530aa0 
Signed-off-by: Sorin Dumitru <sdumitru@bloomberg.net>
azdagron
azdagron reviewed Jun 17, 2024
View reviewed changes
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small nitpick and we're ready to merge. Thank you, @sorindumitru .

conf/server/server_full.conf Outdated Show resolved Hide resolved
@sorindumitru @azdagron
Update conf/server/server_full.conf
d5d871b 
Co-authored-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Sorin Dumitru <sorin@returnze.ro>
@azdagron azdagron added this to the 1.10.1 milestone Jun 18, 2024
@MarcosDY MarcosDY merged commit 60e8844 into spiffe:main Jun 18, 2024
33 checks passed
@kfox1111 kfox1111 mentioned this pull request Jun 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azdagron azdagron azdagron approved these changes

@evan2645 evan2645 Awaiting requested review from evan2645 evan2645 is a code owner

@amartinezfayo amartinezfayo Awaiting requested review from amartinezfayo amartinezfayo is a code owner

@MarcosDY MarcosDY Awaiting requested review from MarcosDY MarcosDY is a code owner

@rturner3 rturner3 Awaiting requested review from rturner3 rturner3 is a code owner

Assignees

@azdagron azdagron

Labels
None yet
Projects
None yet
Milestone
1.10.1
Development

Successfully merging this pull request may close these issues.

Consider adding support for specifying a cert and a key manually for federation endpoint.
3 participants
@sorindumitru @azdagron @MarcosDY