[SECURITY-2053] Prevent plain text credentials of Basic/Digest authentication #105

offa · 2022-07-28T13:05:57Z

Removes Basic / Digest authentication in order to fix SECURITY-2053.

It has been deprecated and marked for removal in 1.8.19+ anyway (we are 1.15 now).

⚠️ There's no migration done for deprecated basic / digest authentications.

Here are my steps used to reproduce the issue and verify my fix:

Reproduce

Install the current version (1.15 or master)
Global settings --> Http request --> Basic/Digest Authentication
Add an entry (incl. username and password)
Save
Verify the plugins XML file contains data of 3.) in plain text

Verify the fix

Update to version of this PR
Basic/Digest Authentication is no longer available through UI (= no new plain credentials possible)
Saving after an edit will remove any existing plain text credentials from the XML

Reviewer

@jenkinsci/http-request-plugin-developers and security team (@Wadeck @daniel-beck)

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

offa · 2022-07-28T13:52:16Z

Related: jenkins-infra/helpdesk#3075

daniel-beck

I would expect this approach to result in warnings about unloadable data.

Might work better from an admin POV to remove all UI and make the field of HttpRequestGlobalConfig transient.

daniel-beck · 2022-07-28T14:55:42Z

src/main/java/jenkins/plugins/http_request/auth/BasicDigestAuthentication.java

-
-	private final String keyName;
-    private final String userName;
-    private final String password;


FTR the only required change was to change this field's type to hudson.util.Secret (ideally also constructor parameter and getter return value), and adapt related code as needed.

Thanks for the review!

The class is marked as deprecated for years and the UI indicates that too. I wouldn't consider this worth the effort of an API change / migration. It's a good chance to remove it for good now. There shouldn't be any use case left … hopefully.

I don't have a strong opinion on this though. So if either making fields transient or migration to Secret are more suitable solution to fix this I can update the PR.

Per the developer documentation:

Removing a field requires that you leave the field with the transient keyword. When Jenkins reads the old XML, XStream will set the value for this field, but it will no longer be written back to XML when data is saved.

Is there any workflow on how to remove a class?

If I understand this correctly transient (all fields?) + UI removal is the preferred solution? PR update is on the way.

Is there any workflow on how to remove a class?

Good question, it's not a case I had to handle before -- usually old types are kept around to do migration of old configuration or data to the new types, and then transient + readResolve do the job.

I'd probably keep the class, and make the field holding it transient. Then the idea would be to make the class an empty shell. I'd try implementing #readObject(ObjectInputStream) or an X-Stream Converter (as a nested class called ConverterImpl) to bypass any warnings from default behaviors, and basically do as little as possible.

(You could customize the deserialization of the class holding the field, but that doesn't seem like a great idea for longer-term maintainability.)

Regarding the comment that started this thread, it was only to point out what a minimal fix would have looked like. We're generally not asking maintainers to rip out entire features, and I did not want this PR to give this impression.

…elds

offa · 2022-07-28T17:26:20Z

PR updated:

Fields are transient
UI element removed

daniel-beck · 2022-07-28T17:38:19Z

FTR the field I would suggest making transient is e24d3b8#diff-e6782195225aa77d34dd0c71b85cabe588c8a7a632b36fb73198a981d63e3e7cL31.

basil

Change is incomplete:

help-userName.html and help-password.html are not shown in the UI but are still present in the repository
We still serialize a <basicDigestAuthentications> when saving an HttpRequestGlobalConfig object; we don't want to serialize anything related to basic digest authentication anymore.
Once the above feedback is addressed by marking the basicDigestAuthentications field as transient, its getters and setters should be deprecated, as calling them will update an in-memory object that will not be persisted
Once the above feedback is addressed usages of the above need to be audited and deleted; we don't want to be invoking a method that will write an object to memory that will then be discarded when the object is serialized
Probably also want to clear any existing deprecated data in memory after reading it from the old version via a readResolve method as described in the developer documentation

offa · 2022-07-28T17:40:08Z

FTR the field I would suggest making transient is e24d3b8#diff-e6782195225aa77d34dd0c71b85cabe588c8a7a632b36fb73198a981d63e3e7cL31.

Ah, I see! This would also get rid of the – now empty – list element. 👍

offa · 2022-07-28T17:42:20Z

Thanks @basil, I'm going to work on your suggestions.

--

Remove help-userName.html and help-password.html
transient basicDigestAuthentications (not BasicDigestAuthentication members)
Deprecate basicDigestAuthentications getter / setter
Audit and delete basicDigestAuthentications usages
Clear existing in memory data

and it's deprecate getter / setter.

offa · 2022-07-28T18:43:02Z

Public:

The setter is used in tests only, getter in tests and filling of the authentication items list box (HttpRequest.DescriptorImpl).

Internal:

Key validation (must be unique) and mapping of key to Authenticator.

basil · 2022-07-29T16:53:42Z

I would expect this approach to result in warnings about unloadable data. Might work better from an admin POV to remove all UI and make the field of HttpRequestGlobalConfig transient.

From another perspective, perhaps unloadable data might be a good UX? Because that way administrators could choose when to delete the plaintext usernames and passwords from disk, whereas without the new approach we would delete the plaintext usernames and passwords from disk when the job is next saved. That might violate the principle of least surprise by deleting data, and perhaps some users would be upset to lose their usernames and passwords without explicitly asking to do so. What do you think @daniel-beck @offa?

offa · 2022-07-29T19:27:39Z

Does unloadable data affect the basic digest elements (= good) or delete the complete data / XML file (= bad)?

In the former case it sounds like a good approach to me 👍.

basil · 2022-07-29T19:39:29Z

Unloadable data doesn't cause any problems for users, but it results in an administrative warning in the Manage Jenkins UI (which has an Old Data page). The administrator than has to press a button to clear the old data. @daniel-beck seems to think that's a bug, but I think it might be considered a feature because it forces the administrator to think about whether they want to erase the old usernames and passwords before they are lost.

daniel-beck · 2022-07-29T20:03:20Z

@daniel-beck seems to think that's a bug, but I think it might be considered a feature because it forces the administrator to think about whether they want to erase the old usernames and passwords before they are lost.

It's usually indicative of a bug in code that was changed incompatibly. Your argument is reasonable though. (IIRC we've never opened up the more user friendly "old data" API to plugins, so it'd be cumbersome to offer a better UX, would need to be a custom admin monitor, and that doesn't seem worth it.) Note however that saving the global config form will drop unloadable data, so it's not really straightforward to retain the data on disk.

offa · 2022-08-01T05:49:53Z

Now, how continue here?

daniel-beck · 2022-08-01T08:42:33Z

Now, how continue here?

Any of the possible approaches are reasonable and have the mentioned pros and cons. The current state looks reasonable to me as far as the global configuration is concerned.

I would look into what happens to job configurations that have this value configured. AFAIUI, it gets set to https://github.com/offa/http-request-plugin/blob/29ff54c72b606473e7b9fa058d6793ec66da2bb0/src/main/java/jenkins/plugins/http_request/HttpRequest.java#L70 and then provided elsewhere to https://github.com/offa/http-request-plugin/blob/29ff54c72b606473e7b9fa058d6793ec66da2bb0/src/main/java/jenkins/plugins/http_request/HttpRequest.java#L199-L201, which may fail or (unexpectedly) continue to work, at least until a Jenkins restart.

I would also remove https://github.com/offa/http-request-plugin/blob/29ff54c72b606473e7b9fa058d6793ec66da2bb0/src/main/java/jenkins/plugins/http_request/HttpRequest.java#L539-L541 as well.

Also consider https://www.jenkins.io/doc/developer/plugin-development/mark-a-plugin-incompatible/ to warn users about the release this change goes it before they update.

offa · 2022-08-01T12:19:28Z

I would look into what happens to job configurations that have this value configured. […] which may fail or (unexpectedly) continue to work, at least until a Jenkins restart.

I did some quick testing with a freestyle job. The job continues to list and use the credentials after switching to this PR version and getting them removed from global config. After a Jenkins restart it jumps to - none - in the jobs UI and build fail with an exception (java.lang.IllegalStateException: Authentication '<Former name here>' doesn't exist anymore).

offa · 2022-08-01T12:35:56Z

I would also remove https://github.com/offa/http-request-plugin/blob/29ff54c72b606473e7b9fa058d6793ec66da2bb0/src/main/java/jenkins/plugins/http_request/HttpRequest.java#L539-L541 as well.

Also consider https://www.jenkins.io/doc/developer/plugin-development/mark-a-plugin-incompatible/ to warn users about the release this change goes it before they update.

Both done, assuming the next release version is 1.16.

daniel-beck · 2022-08-01T12:47:40Z

I would look into what happens to job configurations that have this value configured. […] which may fail or (unexpectedly) continue to work, at least until a Jenkins restart.

I did some quick testing with a freestyle job. The job continues to list and use the credentials after switching to this PR version and getting them removed from global config. After a Jenkins restart it jumps to - none - in the jobs UI and build fail with an exception (java.lang.IllegalStateException: Authentication '<Former name here>' doesn't exist anymore).

Seems tolerable. Depending on how much time you want to invest here, this can probably be done nicer, e.g. telling admins which jobs are losing their configured authentication. But that's potentially a lot more work.

Since I forgot to mention it before and I don't know whether you are aware of this: Please note that the Jenkins security team will not release a new version of this plugin. We're happy to help resolve the issue of course, and we'll update the security warning to mark future releases as fixed, but releasing this plugin must be done by current or future maintainers.

offa · 2022-08-01T13:01:41Z

Seems tolerable. Depending on how much time you want to invest here, this can probably be done nicer, e.g. telling admins which jobs are losing their configured authentication. But that's potentially a lot more work.

Not too much honestly (it's a long time deprecated feature, which isn't used extensively hopefully).

Please note that the Jenkins security team will not release a new version of this plugin.

I know, but at least I can prepare a fix which is confirmed to solve the issue. Btw. thanks a lot at this point @daniel-beck @basil for helping me here!

…e_basicdigestauth

basil

Previous review feedback not addressed: no readResolve, deprecated usages still present, etc.

basil · 2022-08-01T16:20:35Z

src/main/java/jenkins/plugins/http_request/HttpRequestGlobalConfig.java

+     * @deprecated removed without replacement
+     */
+    @Deprecated
+    private transient List<BasicDigestAuthentication> basicDigestAuthentications = new ArrayList<>();


Usage of deprecated field still present in HttpRequestGlobalConfig#getAuthentications

I have removed the usage from getAuthentications(). The other two usages are getter and setter.

What is the best scenario for those? If getter / setter are deprecated but still available I'd suggest keeping the related test cases too (it's still some kind of public API), or remove getter, setter, field (?) and related tests altogether.

Please remove all callers of deprecated methods but retain the methods for binary compatibility

src/main/java/jenkins/plugins/http_request/HttpRequestGlobalConfig.java

offa · 2022-08-01T19:49:20Z

src/main/java/jenkins/plugins/http_request/HttpRequestGlobalConfig.java

@@ -109,7 +109,7 @@ public void setFormAuthentications(
    }

    public List<Authenticator> getAuthentications() {
-		return new ArrayList<>(formAuthentications);
+        return new ArrayList<>(formAuthentications);


The project is configured to use tabs (Intelij + Eclipse settings). :-(

basil

Thanks for the PR!

offa requested a review from a team as a code owner July 28, 2022 13:05

daniel-beck reviewed Jul 28, 2022

View reviewed changes

[SECURITY-2053] Avoid serialization of Basic/Digest authentication fi…

6a41582

…elds

offa force-pushed the security-2053-remove_basicdigestauth branch from e24d3b8 to 6a41582 Compare July 28, 2022 17:25

basil reviewed Jul 28, 2022

View reviewed changes

offa added 2 commits July 28, 2022 19:43

Remove unused help files

fdf1355

Restore BasicDigestAuthentication, transient basicDigestAuthentications

7417ea8

and it's deprecate getter / setter.

offa changed the title ~~[SECURITY-2053] Remove Basic/Digest authentication~~ [SECURITY-2053] Prevent plain text credentials of Basic/Digest authentication Jul 28, 2022

Remove import

29ff54c

offa force-pushed the security-2053-remove_basicdigestauth branch from 1e5984b to 29ff54c Compare July 28, 2022 19:39

offa added 2 commits August 1, 2022 14:21

Remove deprecated authentications from fill items

8638524

Mark version < 1.16 as incompatible

8c5df02

basil added 2 commits August 1, 2022 09:15

Merge remote-tracking branch 'origin/master' into security-2053-remov…

36eb9f0

…e_basicdigestauth

deprecation fixups

d1245bd

basil requested changes Aug 1, 2022

View reviewed changes

offa added 2 commits August 1, 2022 20:01

Remove basicDigestAuthentications from available authentications

fa49be7

Add readResolve() and update test

7c94bf9

offa commented Aug 1, 2022

View reviewed changes

src/main/java/jenkins/plugins/http_request/HttpRequestGlobalConfig.java Outdated Show resolved Hide resolved

offa and others added 3 commits August 1, 2022 20:51

Remove usages of deprecated getter / setter

5bbf87d

fix formatting

f69e060

clear list if non-null

9a9df4d

offa commented Aug 1, 2022

View reviewed changes

basil added the breaking label Aug 1, 2022

basil approved these changes Aug 1, 2022

View reviewed changes

basil merged commit a2cf345 into jenkinsci:master Aug 1, 2022

offa deleted the security-2053-remove_basicdigestauth branch August 2, 2022 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SECURITY-2053] Prevent plain text credentials of Basic/Digest authentication #105

[SECURITY-2053] Prevent plain text credentials of Basic/Digest authentication #105

offa commented Jul 28, 2022

offa commented Jul 28, 2022

daniel-beck left a comment

daniel-beck Jul 28, 2022

offa Jul 28, 2022

basil Jul 28, 2022

offa Jul 28, 2022 •

edited

Loading

daniel-beck Jul 28, 2022 •

edited

Loading

offa commented Jul 28, 2022

daniel-beck commented Jul 28, 2022

basil left a comment

offa commented Jul 28, 2022

offa commented Jul 28, 2022 •

edited

Loading

offa commented Jul 28, 2022 •

edited

Loading

basil commented Jul 29, 2022

offa commented Jul 29, 2022

basil commented Jul 29, 2022

daniel-beck commented Jul 29, 2022

offa commented Aug 1, 2022

daniel-beck commented Aug 1, 2022

offa commented Aug 1, 2022

offa commented Aug 1, 2022

daniel-beck commented Aug 1, 2022

offa commented Aug 1, 2022

basil left a comment

basil Aug 1, 2022

offa Aug 1, 2022 •

edited

Loading

basil Aug 1, 2022

offa Aug 1, 2022 •

edited

Loading

basil left a comment

[SECURITY-2053] Prevent plain text credentials of Basic/Digest authentication #105

[SECURITY-2053] Prevent plain text credentials of Basic/Digest authentication #105

Conversation

offa commented Jul 28, 2022

Reproduce

Verify the fix

Reviewer

offa commented Jul 28, 2022

daniel-beck left a comment

Choose a reason for hiding this comment

daniel-beck Jul 28, 2022

Choose a reason for hiding this comment

offa Jul 28, 2022

Choose a reason for hiding this comment

basil Jul 28, 2022

Choose a reason for hiding this comment

offa Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

daniel-beck Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

offa commented Jul 28, 2022

daniel-beck commented Jul 28, 2022

basil left a comment

Choose a reason for hiding this comment

offa commented Jul 28, 2022

offa commented Jul 28, 2022 • edited Loading

offa commented Jul 28, 2022 • edited Loading

Public:

Internal:

basil commented Jul 29, 2022

offa commented Jul 29, 2022

basil commented Jul 29, 2022

daniel-beck commented Jul 29, 2022

offa commented Aug 1, 2022

daniel-beck commented Aug 1, 2022

offa commented Aug 1, 2022

offa commented Aug 1, 2022

daniel-beck commented Aug 1, 2022

offa commented Aug 1, 2022

basil left a comment

Choose a reason for hiding this comment

basil Aug 1, 2022

Choose a reason for hiding this comment

offa Aug 1, 2022 • edited Loading

Choose a reason for hiding this comment

basil Aug 1, 2022

Choose a reason for hiding this comment

offa Aug 1, 2022 • edited Loading

Choose a reason for hiding this comment

basil left a comment

Choose a reason for hiding this comment

offa Jul 28, 2022 •

edited

Loading

daniel-beck Jul 28, 2022 •

edited

Loading

offa commented Jul 28, 2022 •

edited

Loading

offa commented Jul 28, 2022 •

edited

Loading

offa Aug 1, 2022 •

edited

Loading

offa Aug 1, 2022 •

edited

Loading