JENKINS-56553 Make proxy config compatible with JCasC without workarounds on the plugin side #3935

timja · 2019-03-14T13:25:31Z

See JENKINS-56553.

Proposed changelog entries

Internal: JENKINS-56553 Make ProxyConfiguration compatible with configuration-as-code plugin. Workaround on the JCasC plugin side is no longer required

Submitter checklist

JIRA issue is well described
Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
* Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
Appropriate autotests or explanation to why this change has no tests
It's just getters and setters and requires a plugin dependency to test that configuration as code works, I've ran the test and manually tested it
NA For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@Casz @ndeloof

timja · 2019-03-14T13:27:10Z

core/src/main/java/hudson/ProxyConfiguration.java

     */
-    public String getPassword() {
-        return Secret.toString(secretPassword);
+    public Secret getPassword() {


This breaks compatibility but I don't know of a better way of doing this,
I've ensured that JCasC won't break jenkinsci/configuration-as-code-plugin#769

If this change isn't done, the change functionally works but introduces a security risk in that the password would be exported in plain text.

This change has a compatibility risk but is more secure

From what I can tell, Secret seems to migrate automatically from String. The main backward incompatibility would be if anything calls this method directly and not via Jelly or similar.

Binary compatibility is broken here for an user. There is no automatic type conversion in Java, not sure what @jvz means here. Migration in stapler?

The secret class has a converter that converts strings into secrets automatically.
For a user this change is individual to the best of my knowledge

We really need to retain binary compatibility here somehow

I did that before with the getSecretPassword additional getter which wasn't ideal but @daniel-beck suggested reverting and doing the change as it shouldn't impact anyone?
#3935 (comment)

open to suggestions though

@oleg-nenashev We really don't; I've been unable to identify any plugins using this API.

While we could add a bridge method, I would expect the side effects of that to be potentially far worse than just fixing the return type like this.

jetersen

Change looks good apart from the compatibility but 🤷‍♂️
@oleg-nenashev @jglick ?

daniel-beck · 2019-03-14T18:21:13Z

We don't generally break compatibility except for a very good reason.

Saving 3 lines of code in a plugin is not a very good reason.

Checking all public plugins for calls to the method yields no results -- seems they're all mostly interested in #createProxy. So it might be safe enough to do.

oleg-nenashev

Binary compat issue needs to be fixed, I agree with @daniel-beck's evaluation

timja · 2019-03-18T08:12:04Z

@oleg-nenashev the password variable was deprecated 8 years ago in #130 /
3f13f6d

@daniel-beck evaluated that it was highly unlikely that anyone would be calling it, based on searching all public plugins...

timja · 2019-03-20T07:36:53Z

Compat issue fixed, I've changed it to use secretPassword for JCasC

@oleg-nenashev could you re-review please?

oleg-nenashev

@timja have you verified that password is being passed correctly during the form submission? Unless you change the field in Jelly, it will be trying to pass a secret field which does not longer have databinding

timja · 2019-03-20T08:55:01Z

Thanks for the quick review @oleg-nenashev
I did check that the form worked, but I didn't actually validate the value was correctly saved (given that it comes back as ****'s in the form, I was just assuming it was working, my bad)

You were right it needed an update, it works now:

core/src/main/resources/hudson/ProxyConfiguration/config.groovy

darxriggs · 2019-03-21T22:42:26Z

I suggest to also implement a test for JCasC import & export.
For example see this one for a test that uses one config file to test import & export.
Using Groovy & Spock in this one I find more readable.
If there are better ways I am also interested.

timja · 2019-03-22T06:28:35Z

There's a test in the JCasc plugin, I couldn't see any examples here of core depending on a plugin for testing.
This would be a too recent version of core to be merged to master of JCasC plugin, I can open a PR to show a passing build if you want?

timja · 2019-03-22T14:34:43Z

@darxriggs https://github.com/jenkinsci/configuration-as-code-plugin/pull/784/files There's an example of the test, it's not passing on jenkins as it's ignoring my Jenkinsfile change for the version bump

Tried updating it in the pom file and it kept failing for missing dependencies, added like ~12 and then gave up, seemed like transitive dependencies from core weren't being resolved from incrementals...

timja · 2019-03-24T00:34:28Z

Test demonstrating this working using the incrementals version:
jenkinsci/configuration-as-code-plugin#784

(Couple of unrelated test changes in there to make JCasC pass on higher core version)

timja · 2019-03-25T07:17:50Z

@oleg-nenashev could you re-review when you have a chance please?

timja · 2019-03-29T11:26:05Z

Nudge: @oleg-nenashev please?

daniel-beck · 2019-04-09T21:08:02Z

FWIW I would be fine with just changing the signature of the password stuff since that's not actually used anywhere. I apologize for the unclear comment I left, it sort of evolved after I checked the plugin ecosystem for actual uses of this.

timja · 2019-04-10T20:30:48Z

JCasC PR updated pointing at the new incrementals and it has a green build:
jenkinsci/configuration-as-code-plugin#784

…enkins into JENKINS-56553-proxy-config-jcasc

timja · 2019-10-18T14:37:45Z

Code looks good. Has any automated test being implemented, maybe on JCasC side?

@varyvol tests pass now on JCASC PR: jenkinsci/configuration-as-code-plugin#784
(had to make some changes to fix upper bounds and trilead-api failures bumping the core version to 2.201)

varyvol · 2019-10-18T14:48:02Z

@timja fine by me, I had already approved, I just was suggesting some test coverage in case there wasn't, which is not the case.

timja · 2019-10-18T15:01:49Z

CI seems flakey today... (note this build has previously been green with no code changes), i.e. see the green incrementals check

timja · 2019-10-18T15:02:43Z

4 approvals, adding ready for merge...

timja · 2019-10-31T10:25:57Z

@oleg-nenashev this is marked as you requesting a change, this was addressed.
Would you be able to take another look please or dismiss your review?

timja · 2019-11-12T14:20:11Z

@oleg-nenashev you able to merge this?

oleg-nenashev

Looks right.
I plan to merge it tomorrow if no negative feedback

daniel-beck

This caused JENKINS-61692.

daniel-beck · 2020-03-28T00:08:13Z

core/src/main/resources/hudson/ProxyConfiguration/config.groovy

@@ -11,8 +11,8 @@ f.entry(title:_("Port"),field:"port") {
 f.entry(title:_("User name"),field:"userName") {
    f.textbox()
 }
-f.entry(title:_("Password"),field:"password") {
-    f.password(value:instance?.encryptedPassword)
+f.entry(title:_("Password"),field:"secretPassword") {


Field renamed without adapting validateButton arguments in https://github.com/jenkinsci/jenkins/pull/3935/files#diff-4af47ca230111a1e83ea06084040271fR25

Fix attempt in 8dcf818

daniel-beck · 2020-03-28T00:10:42Z

core/src/main/java/hudson/ProxyConfiguration.java

    public ProxyConfiguration(String name, int port, String userName, String password, String noProxyHost, String testUrl) {
        this.name = Util.fixEmptyAndTrim(name);
        this.port = port;
        this.userName = Util.fixEmptyAndTrim(userName);
-        this.secretPassword = Secret.fromString(password);
+        String tempPassword = Util.fixEmptyAndTrim(password);


Better hope actual passwords never have whitespace at the beginning or end.

Yeah I generally do not recommend trimming input unless you specifically know that leading/trailing whitespace could not be legal.

JENKINS-56553 Make proxy config compatible with JCASC

92ca592

timja commented Mar 14, 2019

View reviewed changes

jetersen approved these changes Mar 14, 2019

View reviewed changes

daniel-beck added needs-justification This PR lacks a motivation for the proposed change. and removed needs-justification This PR lacks a motivation for the proposed change. labels Mar 14, 2019

oleg-nenashev requested changes Mar 18, 2019

View reviewed changes

Use DataBoundSetters as other fields are optional

6543d7a

oleg-nenashev requested changes Mar 20, 2019

View reviewed changes

Fix Jelly for ProxyConfiguration

e03a344

darxriggs reviewed Mar 21, 2019

View reviewed changes

core/src/main/resources/hudson/ProxyConfiguration/config.groovy Outdated Show resolved Hide resolved

Use default field name now that field matches

774ce1e

darxriggs reviewed Mar 21, 2019

View reviewed changes

core/src/main/resources/hudson/ProxyConfiguration/config.groovy Show resolved Hide resolved

timja added 2 commits March 22, 2019 07:29

Merge branch 'master' into JENKINS-56553-proxy-config-jcasc

37476b3

Fix validate proxy

2f6b64a

timja mentioned this pull request Mar 22, 2019

🚧 Update proxy config jenkinsci/configuration-as-code-plugin#784

Closed

4 tasks

timja added 2 commits April 10, 2019 08:36

Merge branch 'master' into JENKINS-56553-proxy-config-jcasc

559eb60

Change field type

8a11f1a

daniel-beck assigned oleg-nenashev Apr 10, 2019

Merge branch 'JENKINS-56553-proxy-config-jcasc' of github.com:timja/j…

ebaeb69

…enkins into JENKINS-56553-proxy-config-jcasc

timja closed this Oct 18, 2019

timja reopened this Oct 18, 2019

timja closed this Oct 18, 2019

timja reopened this Oct 18, 2019

timja added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Oct 18, 2019

timja closed this Oct 19, 2019

timja reopened this Oct 19, 2019

timja closed this Oct 30, 2019

timja reopened this Oct 30, 2019

timja closed this Oct 30, 2019

timja reopened this Oct 30, 2019

timja closed this Nov 10, 2019

timja reopened this Nov 10, 2019

oleg-nenashev approved these changes Nov 12, 2019

View reviewed changes

oleg-nenashev added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Nov 13, 2019

oleg-nenashev changed the title ~~JENKINS-56553 Make proxy config compatible with JCASC~~ JENKINS-56553 Make proxy config compatible with JCasC without workarounds on the plugin side Nov 13, 2019

oleg-nenashev merged commit f103b78 into jenkinsci:master Nov 14, 2019

timja mentioned this pull request Jan 30, 2020

[JENKINS-51856] Apply configuration before the jobs are loaded jenkinsci/configuration-as-code-plugin#1262

Merged

6 tasks

daniel-beck mentioned this pull request Mar 16, 2020

Update at-since up to and including 2.226 #4587

Merged

10 tasks

daniel-beck reviewed Mar 28, 2020

View reviewed changes

jglick mentioned this pull request Feb 12, 2024

Basic ProxyConfigurationManagerTest #8952

Merged

jglick mentioned this pull request Feb 24, 2024

Null out ProxyConfiguration.userName on save #8990

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JENKINS-56553 Make proxy config compatible with JCasC without workarounds on the plugin side #3935

JENKINS-56553 Make proxy config compatible with JCasC without workarounds on the plugin side #3935

timja commented Mar 14, 2019 •

edited by oleg-nenashev

Loading

timja Mar 14, 2019

jvz Mar 15, 2019

oleg-nenashev May 8, 2019

timja May 8, 2019

oleg-nenashev May 8, 2019

timja May 8, 2019

daniel-beck May 10, 2019 •

edited

Loading

jetersen left a comment •

edited

Loading

daniel-beck commented Mar 14, 2019 •

edited

Loading

oleg-nenashev left a comment

timja commented Mar 18, 2019 •

edited

Loading

timja commented Mar 20, 2019 •

edited

Loading

oleg-nenashev left a comment

timja commented Mar 20, 2019 •

edited

Loading

darxriggs commented Mar 21, 2019 •

edited

Loading

timja commented Mar 22, 2019

timja commented Mar 22, 2019

timja commented Mar 24, 2019

timja commented Mar 25, 2019

timja commented Mar 29, 2019

daniel-beck commented Apr 9, 2019

timja commented Apr 10, 2019 •

edited

Loading

timja commented Oct 18, 2019

varyvol commented Oct 18, 2019

timja commented Oct 18, 2019 •

edited

Loading

timja commented Oct 18, 2019

timja commented Oct 31, 2019

timja commented Nov 12, 2019

oleg-nenashev left a comment

daniel-beck left a comment

daniel-beck Mar 28, 2020

daniel-beck Mar 28, 2020

jglick Mar 30, 2020

JENKINS-56553 Make proxy config compatible with JCasC without workarounds on the plugin side #3935

JENKINS-56553 Make proxy config compatible with JCasC without workarounds on the plugin side #3935

Conversation

timja commented Mar 14, 2019 • edited by oleg-nenashev Loading

Proposed changelog entries

Submitter checklist

Desired reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-beck May 10, 2019 • edited Loading

Choose a reason for hiding this comment

jetersen left a comment • edited Loading

Choose a reason for hiding this comment

daniel-beck commented Mar 14, 2019 • edited Loading

oleg-nenashev left a comment

Choose a reason for hiding this comment

timja commented Mar 18, 2019 • edited Loading

timja commented Mar 20, 2019 • edited Loading

oleg-nenashev left a comment

Choose a reason for hiding this comment

timja commented Mar 20, 2019 • edited Loading

darxriggs commented Mar 21, 2019 • edited Loading

timja commented Mar 22, 2019

timja commented Mar 22, 2019

timja commented Mar 24, 2019

timja commented Mar 25, 2019

timja commented Mar 29, 2019

daniel-beck commented Apr 9, 2019

timja commented Apr 10, 2019 • edited Loading

timja commented Oct 18, 2019

varyvol commented Oct 18, 2019

timja commented Oct 18, 2019 • edited Loading

timja commented Oct 18, 2019

timja commented Oct 31, 2019

timja commented Nov 12, 2019

oleg-nenashev left a comment

Choose a reason for hiding this comment

daniel-beck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timja commented Mar 14, 2019 •

edited by oleg-nenashev

Loading

daniel-beck May 10, 2019 •

edited

Loading

jetersen left a comment •

edited

Loading

daniel-beck commented Mar 14, 2019 •

edited

Loading

timja commented Mar 18, 2019 •

edited

Loading

timja commented Mar 20, 2019 •

edited

Loading

timja commented Mar 20, 2019 •

edited

Loading

darxriggs commented Mar 21, 2019 •

edited

Loading

timja commented Apr 10, 2019 •

edited

Loading

timja commented Oct 18, 2019 •

edited

Loading