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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-62375] Fix/secret token #91

Merged
merged 9 commits into from
May 31, 2020
Merged

[JENKINS-62375] Fix/secret token #91

merged 9 commits into from
May 31, 2020

Conversation

baymac
Copy link
Member

@baymac baymac commented May 21, 2020

  1. Add a secret token field to GitLab Server entry
  2. Add secret token while creating webhook/system hook
  3. Validating secret token in WebhookAction
  4. Modify webhook url if secret token in url doesn't match configured secret token (for legacy reason)
  5. Would not modify systemhook url if secret token inconsistent bcz GitLab4J api doesn't support it. It would be fairly easy to setup so expecting user to modify their systemhook url manually.

https://issues.jenkins-ci.org/browse/JENKINS-62375

jetersen
jetersen requested changes May 21, 2020
View reviewed changes
src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServer.java Outdated
Comment on lines 258 to 263
private void generateSecretToken() {
byte[] random = new byte[16]; // 16x8=128bit worth of randomness, since we use md5 digest as the API token
RANDOM.nextBytes(random);
this.secretToken = Util.toHexString(random);
}

Copy link
Member

Choose a reason for hiding this comment

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

This us unused.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, will be used by some UI element to trigger this method, thats a TODO on me.

src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServer.java Outdated Show resolved Hide resolved
src/main/java/io/jenkins/plugins/gitlabbranchsource/GitLabSystemHookAction.java Outdated
Comment on lines 85 to 93
private boolean isValidToken(String secretToken) {
List<GitLabServer> servers = GitLabServers.get().getServers();
for(GitLabServer server: servers) {
if(server.getSecretToken().equals(secretToken)) {
return true;
}
}
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

This can easily be deduped.

src/main/java/io/jenkins/plugins/gitlabbranchsource/GitLabSystemHookAction.java
Comment on lines +73 to +77
String secretToken = request.getHeader("X-Gitlab-Token");
if(!isValidToken(secretToken)) {
return HttpResponses.error(HttpServletResponse.SC_UNAUTHORIZED,
"Expecting a valid secret token");
}
Copy link
Member

Choose a reason for hiding this comment

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

This can also be deduped.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean by that?

src/main/java/io/jenkins/plugins/gitlabbranchsource/GitLabHookCreator.java Outdated Show resolved Hide resolved
src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServer.java Outdated Show resolved Hide resolved
src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServer.java Outdated Show resolved Hide resolved
src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServer.java Outdated Show resolved Hide resolved
src/main/resources/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServer/config.groovy Outdated Show resolved Hide resolved
src/main/java/io/jenkins/plugins/gitlabbranchsource/GitLabSystemHookAction.java Outdated
private boolean isValidToken(String secretToken) {
List<GitLabServer> servers = GitLabServers.get().getServers();
for(GitLabServer server: servers) {
if(server.getSecretToken().equals(secretToken)) {
Copy link
Member

Choose a reason for hiding this comment

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

Might not be the correct way as there are potential null pointers lurking.

Suggested change
if(server.getSecretToken().equals(secretToken)) {
if(server.getSecretToken().getPlainValue().equals(secretToken)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding a try catch should solve this?

@baymac
Copy link
Member Author

baymac commented May 21, 2020

Screenshot 2020-05-22 at 12 36 56 AM

When I add a new instance of GitLab Server in the UI and refresh, the secret token field contains some fixed encrypted_password like characters. Is this expected? Should it be empty or that's how empty secret looks like? @jetersen

@jetersen
Copy link
Member

if manage hooks are enabled they do not need to know the secret token.

if manage hooks are disabled they can manually add a secret token and than they would have to go to gitlab and manually input that token.

@baymac
Copy link
Member Author

baymac commented May 22, 2020

@jetersen I attempted to create a generateSecretToken button that sets the secret token field but since we are using JS to add value to the field, the token value is null. Can we revert back to String field instead of Secret? Is there a Java way to populate TextField on button click?

@baymac baymac added the enhancement New feature or request label May 24, 2020
@jetersen
Copy link
Member

This change forces secret token to be set. This definitely seems like a breaking change.

@baymac baymac requested a review from jetersen May 28, 2020 00:26
@baymac baymac merged commit 66f422b into master May 31, 2020
@baymac baymac deleted the fix/secret-token branch May 31, 2020 12:24
dubrsl
dubrsl reviewed Oct 15, 2020
View reviewed changes
src/main/java/io/jenkins/plugins/gitlabbranchsource/GitLabHookCreator.java
@@ -84,14 +84,15 @@ public static void register(GitLabSCMSource source,
return;
}
String hookUrl = getHookUrl(server, true);
String secretToken = server.getSecretToken().getPlainText();
Copy link
Contributor

Choose a reason for hiding this comment

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

If secretToken is null it leads to Exception https://pastebin.com/ptxv5DLz

Copy link
Member

Choose a reason for hiding this comment

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

I created a PR against this. #105
Please feel free to help me to review it.

@TBBle TBBle mentioned this pull request Jan 28, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dubrsl dubrsl dubrsl left review comments

@LinuxSuRen LinuxSuRen LinuxSuRen left review comments

@markjacksonfishing markjacksonfishing Awaiting requested review from markjacksonfishing

@jetersen jetersen Awaiting requested review from jetersen jetersen is a code owner

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@baymac @jetersen @dubrsl @LinuxSuRen