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

[JENKINS-58743] Allow to provide a custom path for master key #10235

Merged

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Feb 4, 2025

See JENKINS-58743.

The following system properties can be used to control the behaviour

  • jenkins.security.DefaultConfidentialStore.file allows to provide an alternative path to load/store the master key (when unset, defaults to $JENKINS_HOME/secrets/master.key as previously)
  • jenkins.security.DefaultConfidentialStore.readOnly when true, prevents Jenkins from generating a master key. If the master key file doesn't exist, Jenkins fails to start.

User-facing doc jenkins-infra/jenkins.io#7859

Testing done

Create master.key beforehand in standard path
export JENKINS_HOME=/tmp/cc
mkdir -p $JENKINS_HOME/secrets
openssl rand -hex 128 >$JENKINS_HOME/secrets/master.key
java -Djenkins.master.key.readOnly=true -jar war/target/jenkins.war

✅ Normal startup sequence

Content of $JENKINS_HOME/secrets after startup

initialAdminPassword  jenkins.model.Jenkins.crumbSalt  master.key
Create master.key beforehand in custom path
export JENKINS_HOME=/tmp/cc
masterkey=$(mktemp)
openssl rand -hex 128 >$masterkey
java -Djenkins.master.key.readOnly=true -Djenkins.master.key.file=$masterkey -jar war/target/jenkins.war

✅ Normal startup sequence

Content of $JENKINS_HOME/secrets after startup

initialAdminPassword  jenkins.model.Jenkins.crumbSalt
Readonly missing master key

Starting with -Djenkins.master.key.readOnly=true and missing master.key.

Jenkins fails to start

[...]
2025-02-05 11:28:05.541+0000 [id=40]    INFO    hudson.lifecycle.Lifecycle#onStatusUpdate: Jenkins stopped
Exception in thread "Jenkins initialization thread" java.lang.Error: java.io.IOException: /tmp/cc/secrets/master.key does not exist and system property jenkins.security.DefaultConfidentialStore.readOnly is set. You must provide a valid master key file.
        at jenkins.security.ConfidentialStore.get(ConfidentialStore.java:93)
        at jenkins.model.Jenkins.<init>(Jenkins.java:977)
        at hudson.model.Hudson.<init>(Hudson.java:102)
        at hudson.model.Hudson.<init>(Hudson.java:87)
        at hudson.WebAppMain$3.run(WebAppMain.java:249)
Caused by: java.io.IOException: /tmp/cc/secrets/master.key does not exist and system property jenkins.security.DefaultConfidentialStore.readOnly is set. You must provide a valid master key file.
        at jenkins.security.DefaultConfidentialStore.<init>(DefaultConfidentialStore.java:85)
        at jenkins.security.DefaultConfidentialStore.<init>(DefaultConfidentialStore.java:71)
        at jenkins.security.DefaultConfidentialStore.<init>(DefaultConfidentialStore.java:67)
        at jenkins.security.ConfidentialStore.get(ConfidentialStore.java:90)
        ... 4 more

Proposed changelog entries

  • Allow the master key to be stored in a separate location.

Proposed upgrade guidelines

N/A

Submitter checklist

Preview Give feedback

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Preview Give feedback

@Vlatombe Vlatombe requested a review from jglick February 4, 2025 09:18
@Vlatombe Vlatombe requested a review from a team February 4, 2025 14:13
@jglick
Copy link
Member

jglick commented Feb 4, 2025

Jenkins doesn't fail to load

Insert a call to ConfidentialStore.get into the startup sequence somewhere?

if (SystemProperties.getBoolean(MASTER_KEY_READONLY_SYSTEM_PROPERTY_NAME)) {
throw new IOException(masterSecret + " does not exist and system property " + MASTER_KEY_READONLY_SYSTEM_PROPERTY_NAME + " is set. You must provide a valid master key file.");
} else {
// we are only going to use small number of bits (since export control limits AES key length)
Copy link
Member

Choose a reason for hiding this comment

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

BTW I think this comment has been obsolete for years?

Copy link
Member Author

Choose a reason for hiding this comment

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

Still

// Due to the stupid US export restriction JDK only ships 128bit version.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, though would take non-trivial changes to migrate to the new key length. Not in scope of my current change.

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Feb 5, 2025
@krisstern krisstern merged commit 314930c into jenkinsci:master Feb 6, 2025
16 checks passed
@Vlatombe Vlatombe deleted the JENKINS-58743-external-master-key branch February 7, 2025 08:07
@daniel-beck
Copy link
Member

Please file a pull request to document these system properties at https://www.jenkins.io/doc/book/managing/system-properties/

@Vlatombe
Copy link
Member Author

Filed jenkins-infra/jenkins.io#7859

@MarkEWaite MarkEWaite added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants