-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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-61808] Always transmit f:password values as Secret #4630
Conversation
IIUC this means that a plugin which whose |
Unsure what you mean. AFAICT, there is no It requires a manually set up configuration that As I wrote, if we think there's value in having the test specific behavior, I can try to keep it around and just disable it for the newly added tests only; but I don't see a lot of value here. |
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.
LGTM
@@ -0,0 +1 @@ | |||
hidden=(password value not shown) |
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 page intentionally left blank
The code looks sensible, I would like to do some interactive testing before approving, but don't take that as a blocker if others approve this and are happy |
/* Log a warning if we're in development mode (core or plugin): There's an f:password backed by a non-Secret */ | ||
if (req != null && (Boolean.getBoolean("hudson.hpi.run") || Boolean.getBoolean("hudson.Main.development"))) { | ||
LOGGER.log(Level.WARNING, () -> "<f:password/> form control in " + getJellyViewsInformationForCurrentRequest() + | ||
" is not backed by hudson.util.Secret. Learn more: https://jenkins.io/redirect/hudson.util.Secret"); |
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.
shouldn't this redirect be pointing here https://www.jenkins.io/doc/developer/security/secrets/ instead of the wiki?
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.
Yup, the redirect target is terrible, needs to be changed. But not worth doing another redirect for it. We probably need a new docs page to talk about this + toString
.
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.
Tested:
- adding a proxy with password in this PR, all worked fine, and no log messages
- hacking proxyconfiguration to be backed by a string, and strings getter/setter, got expected warning message with view:
Apr 30, 2020 8:55:28 PM hudson.Functions getPasswordValue
WARNING: <f:password/> form control in PluginManager/advanced.jelly is not backed by hudson.util.Secret. Learn more: https://jenkins.io/redirect/hudson.util.Secret
- string field, with secret getters and setters - no log message and field stored in plain text (Not handled but probably unlikely to happen)
LGTM
On-holding until after 2.235, just in case. |
Rather than the |
Not in progress though, might send the wrong signal to reviewers. |
Removed on-hold as 2.235 released |
This PR is now ready for merge, let's merge it tomorrow if there's no negative feedback |
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.
I deployed the patch on my test instance, and it looks to be OK.
I also do not see anything wrong in the code. Will proceed with merge
Caused JENKINS-62305. |
See JENKINS-61808.
The
<f:password/>
form control has a magic behavior, provided byFunctions#getPasswordValue
and a converter inSecret.java
:If it's backed by a
Secret
, it will use the encrypted form of the password (or token, key, etc.) asvalue
, rather than the plain text.Additionally, if it's backed by a
Secret
, anItem
is in the URL hierarchy, and the user lacks Item/Configure, it will only receive******
to be rendered, instead of a real value.The latter is less important since Jenkins 2.223, which introduced read-only forms for users with Item/ExtendedRead, and a representation of
f:password
that doesn't even attempt to render a real value.Unfortunately, many plugin maintainers get this wrong and use
String
-based for values only shown using<f:password/>
. This affects both plugins who get storage right (usingSecret
), and those that get that part wrong, too.So what does this do?
<f:password/>
, even if it's not backed by aSecret
, we will convert the value to be shown to aSecret
and render its encrypted form in most cases.Additionally, we also allow non-
Secret
backed<f:password/>
to be******
out when the user lacks a relevant Item/Configure permission.Converter
will convert any value that's an encryptedSecret
to its decryptedString
representation.In addition to the above, we also add a new warning to the log whenever a
<f:password/>
is backed by a string, and attempt to identify which view it's coming from (which is surprisingly annoying to do). This PR does not mean that plugins havingString
APIs wouldn't be a bug. This warning will appear when runningjetty:run
andhpi:run
, but not in production use. It can also serve as a useful reminder that the underlying field should be aSecret
.Since the APIs for password parameters were using the
String
type, they have been adapted in this PR.Escape hatches
This PR introduce more magic, so we have two different escape hatches in case things go sideways:
hudson.util.Secret.AUTO_ENCRYPT_PASSWORD_CONTROL
istrue
by default and controls whether the basic new behavior is active. This must be set on startup, as we don't even want the converter to register if something goes wrong.hudson.util.Secret.BLANK_NONSECRET_PASSWORD_FIELDS_WITHOUT_ITEM_CONFIGURE
istrue
by default and controls whether the*****
masking that existed forSecret
backedf:password
fields is extended to those without aSecret
.Limitations
This introduces a slight regression with
String
-based "setter" APIs around aSecret
field: A round-trip will now result in changes to the persisted value, as Jenkins transparently decrypts any submitted value. IOW, it now behaves as though the user always entered the same value manually.In development mode, Jenkins will log warnings when it encounters secrets not backed by a
Secret
, including fields showing a fixedString
value that's not protection-worthy. This should rarely occur legitimately. It would occur on "Build With Parameters" with password parameters, which use a fixed<DEFAULT>
placeholder, if we didn't specifically support that ingetPasswordValue
, and it happened in the "Parameters" action of builds when not showing a password value, so I replaced the (inappropriate) use off:password
there.If the storage of credentials is wrong, rather than just the Java API exposing values to form fields, then users can still obtain the plain-text stored credential using
GET config.xml
. But plain-text storage of credentials is a problem independent of the UI and remote API to begin with and would need to be addressed anyway.This PR removes a protection that would make (some) unit tests fail:
jenkins/core/src/main/java/hudson/Functions.java
Lines 2027 to 2029 in 449c5ac
We can probably restore that if desired, and set a flag to ignore that for these specific tests, but TBH I'm not sure how much this helps anyway.
Proposed changelog entries
Secret
field. In case of problems, this can be disabled by setting the system propertyhudson.util.Secret.AUTO_ENCRYPT_PASSWORD_CONTROL
tofalse
on startup.Secret
field. In case of problems, this can be disabled by setting the system propertyhudson.util.Secret.BLANK_NONSECRET_PASSWORD_FIELDS_WITHOUT_ITEM_CONFIGURE
tofalse
.Or we can limit the details we expose to users here and just go with
If there are real problems, we can provide the system properties in regression reports.
Proposed upgrade guidelines
N/A
Submitter checklist
Proposed changelog entries
section only if there are breaking changes or other changes which may require extra steps from users during the upgradeDesired reviewers
@mention
Maintainer checklist
Before the changes are marked as
ready-for-merge
:Proposed changelog entries
are correctupgrade-guide-needed
label is set and there is aProposed upgrade guidelines
section in the PR title. (example)lts-candidate