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

Store encrypted password #157

Merged
merged 2 commits into from May 27, 2014

Conversation

Projects
None yet
5 participants
@rinrinne
Copy link
Member

commented May 27, 2014

Now password for SSH authentication file is stored as plain text.

This patch fixes it. Already stored password would be replaced to
encrypted ones if config is saved once.

Fix for JENKINS-23165

Store encrypted password
Now password for SSH authentication file is stored as plain text.

This patch fixes it. Already stored password would be replaced to
encrypted ones if config is saved once.

Fix for JENKINS-23165
@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented May 27, 2014

plugins » gerrit-trigger-plugin #276 UNSTABLE
Looks like there's a problem with this pull request

@rsandell

This comment has been minimized.

Copy link
Member

commented May 27, 2014

How would this handle reading an old configuration that stores the password in clear text of the same member var?

You might need to create a new field with a slightly different name to be the Secret and then readResolve the old but now transient field.

@rinrinne

This comment has been minimized.

Copy link
Member Author

commented May 27, 2014

Secret.fromString() can take both plain text and encrypted text. And Secret instance keeps plain(decrypted) data. So this code can read old password.

In ConfigTest, existing test gives plain text password. Added testGetGerritAuthKeyFilePasswordFromEncryptedString() gives encrypted ones. both works fine.

So we can reuse the same field even if change type because stored data in config.xml has no type information.

If any concern, Please read Secret.java in core:
https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/util/Secret.java

@rsandell

This comment has been minimized.

Copy link
Member

commented May 27, 2014

OK, so why all the NPEs in CredentialStore for the failed tests?

Fix tests depends on Config class
To use Config class, jenkins instance is needed.

And add PowerMockIgnore annotation to prevent ClassCastException.
@rinrinne

This comment has been minimized.

Copy link
Member Author

commented May 27, 2014

Now new patch is uploaded. To create Config instance, Jenkins instance is also needed. But I forgot to add it to some test classes. Sorry...

But ClassCastException was raised even if Jenkins instance is added. it was caused by powermock. So adds annotation PowerMockIgnore for javax.crypto.*.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented May 27, 2014

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented May 27, 2014

plugins » gerrit-trigger-plugin #277 SUCCESS
This pull request looks good

rsandell added a commit that referenced this pull request May 27, 2014

@rsandell rsandell merged commit 351b4fb into jenkinsci:master May 27, 2014

authentication = new Authentication(gerritAuthKeyFile, gerritUserName, "");
} else {
authentication = new Authentication(
gerritAuthKeyFile, gerritUserName, gerritAuthKeyFilePassword.getPlainText());

This comment has been minimized.

Copy link
@jglick

jglick May 27, 2014

Member

Easier to use Secret.toString(gerritAuthKeyFilePassword).

This comment has been minimized.

Copy link
@rinrinne

rinrinne May 27, 2014

Author Member

Secret.toString() is deprecated. Please check the code of Secret class.

This comment has been minimized.

Copy link
@jglick

jglick May 27, 2014

Member

toString() is deprecated. I was suggesting toString(Secret). Reread my comment.

return "";
} else {
return gerritAuthKeyFilePassword.getPlainText();
}
}

This comment has been minimized.

Copy link
@jglick

jglick May 27, 2014

Member

I think this is wrong. The getter should return Secret. The reason is that in GerritServer/index.jelly,

value="${it.config.gerritAuthKeyFilePassword}"

will load the password into the form in plain text. Secret is designed to be passed back to form reconfiguration in encrypted form.

rinrinne added a commit to rinrinne/gerrit-trigger-plugin that referenced this pull request May 28, 2014

@rinrinne rinrinne deleted the rinrinne:encrypt-pw branch Jun 17, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.