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

Fix docker-compose service order #1537

Merged
merged 1 commit into from
Jan 26, 2024
Merged

Conversation

cmurphy
Copy link
Contributor

@cmurphy cmurphy commented Jan 25, 2024

Without this change, when Fulcio is run in docker-compose, signing an artifact may fail with the error "Error entering certificate in CTL". This happens if the docker-compose service have been run previously on the host and the ctfeConfig volume is populated from the last run, so it would generally only be seen in a developer environment. The error happens because the ctfe_init container starts too soon, and ct_server starts with Fulcio's ephemeral root CA from the last run, which is now the wrong CA. This change fixes the issue by ensuring ct_server only starts after ctfe_init has exited successfully, instead of just after it is started. This also means that Fulcio needs to be one of the first services to start so that it can make the ephemeral CA available to download.

Summary

Release Note

NONE

Documentation

Without this change, when Fulcio is run in docker-compose, signing an
artifact may fail with the error "Error entering certificate in CTL".
This happens if the docker-compose service have been run previously on
the host and the ctfeConfig volume is populated from the last run, so
it would generally only be seen in a developer environment. The error
happens because the ctfe_init container starts too soon, and ct_server
starts with Fulcio's ephemeral root CA from the last run, which is now
the wrong CA. This change fixes the issue by ensuring ct_server only
starts after ctfe_init has exited successfully, instead of just after it
is started. This also means that Fulcio needs to be one of the first
services to start so that it can make the ephemeral CA available to
download.

Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
Copy link

codecov bot commented Jan 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ed48e3a) 57.58% compared to head (bcbf96a) 57.74%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1537      +/-   ##
==========================================
+ Coverage   57.58%   57.74%   +0.16%     
==========================================
  Files          50       50              
  Lines        3112     3112              
==========================================
+ Hits         1792     1797       +5     
+ Misses       1160     1156       -4     
+ Partials      160      159       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cmurphy cmurphy marked this pull request as draft January 25, 2024 19:45
@cmurphy cmurphy marked this pull request as ready for review January 25, 2024 23:49
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

I wonder if we should also be enforcing that the CT log is healthy when starting up the service, here:

fulcio/cmd/app/serve.go

Lines 262 to 277 in b7b2eba

if logURL := viper.GetString("ct-log-url"); logURL != "" {
opts := jsonclient.Options{
Logger: logAdaptor{logger: log.Logger},
}
// optionally add CT log public key to verify SCTs
if pubKeyPath := viper.GetString("ct-log-public-key-path"); pubKeyPath != "" {
pemPubKey, err := os.ReadFile(filepath.Clean(pubKeyPath))
if err != nil {
log.Logger.Fatal(err)
}
opts.PublicKey = string(pemPubKey)
}
ctClient, err = ctclient.New(logURL, &http.Client{Timeout: 30 * time.Second}, opts)
if err != nil {
log.Logger.Fatal(err)
}

This would prevent Fulcio starting up while the CT log failed to, and all requests failing.

We would have a chicken-and-egg problem though - Fulcio requiring the CT log, the CT log init script requiring Fulcio. We could generate the CA certificate out of band and give that to the CT log rather than use the init script, though we'll need another script to generate fresh CA certs.

Anywho, not a blocker, just something we could change later on.

@cmurphy
Copy link
Contributor Author

cmurphy commented Jan 26, 2024

We would have a chicken-and-egg problem though

Right, this wouldn't be possible with the way we're currently running fulcio with --ca=ephemeralca in docker-compose. If we were using a different signing backend for development then we could pass the CA to the CT log out of band and we wouldn't have this ordering issue. Since we're not, fulcio has to be up before the CT log can start, at least for this use case.

@haydentherapper haydentherapper merged commit 607d673 into sigstore:main Jan 26, 2024
13 checks passed
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