[JENKINS-62375] Fix/secret token #91

baymac · 2020-05-21T01:16:33Z

Add a secret token field to GitLab Server entry
Add secret token while creating webhook/system hook
Validating secret token in WebhookAction
Modify webhook url if secret token in url doesn't match configured secret token (for legacy reason)
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 · 2020-05-21T07:36:24Z

src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServer.java

+    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);
+    }
+


This us unused.

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

jetersen · 2020-05-21T07:40:56Z

src/main/java/io/jenkins/plugins/gitlabbranchsource/GitLabSystemHookAction.java

+    private boolean isValidToken(String secretToken) {
+        List<GitLabServer> servers = GitLabServers.get().getServers();
+        for(GitLabServer server: servers) {
+            if(server.getSecretToken().equals(secretToken)) {
+                return true;
+            }
+        }
+        return false;
+    }


This can easily be deduped.

jetersen · 2020-05-21T07:41:38Z

src/main/java/io/jenkins/plugins/gitlabbranchsource/GitLabSystemHookAction.java

+        String secretToken = request.getHeader("X-Gitlab-Token");
+        if(!isValidToken(secretToken)) {
+            return HttpResponses.error(HttpServletResponse.SC_UNAUTHORIZED,
+                "Expecting a valid secret token");
+        }


This can also be deduped.

Not sure what you mean by that?

src/main/java/io/jenkins/plugins/gitlabbranchsource/GitLabHookCreator.java

src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServer.java

src/main/resources/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServer/config.groovy

jetersen · 2020-05-21T07:48:17Z

src/main/java/io/jenkins/plugins/gitlabbranchsource/GitLabSystemHookAction.java

+    private boolean isValidToken(String secretToken) {
+        List<GitLabServer> servers = GitLabServers.get().getServers();
+        for(GitLabServer server: servers) {
+            if(server.getSecretToken().equals(secretToken)) {


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)) {

Adding a try catch should solve this?

…Joseph suggestion

baymac · 2020-05-21T19:08:55Z

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 · 2020-05-21T19:23:43Z

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.

…from JS

baymac · 2020-05-22T00:00:35Z

@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?

jetersen · 2020-05-24T14:04:15Z

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

dubrsl · 2020-10-15T00:49:40Z

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();


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

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

baymac added 2 commits May 21, 2020 06:05

Add secret token to webhook creation and validation to WebhookAction

267412c

Add modify ste createMissingHookUrl

97cc1e9

baymac requested review from jetersen and markjacksonfishing May 21, 2020 01:16

jetersen requested changes May 21, 2020

View reviewed changes

baymac added 3 commits May 21, 2020 18:23

Make secretToken of type Secret, check NPE, fix minor bugs (based on …

97f7cec

…Joseph suggestion

Fix static bugs

837d94c

Remove static RANDOM variable, move inside generateSecretToken

0c4b2c2

Add documentation

ad92e3f

Add generate secret token but since secret field so unable to set it …

c9ebaf6

…from JS

baymac added the enhancement New feature or request label May 24, 2020

Abandon efforts to add generate button to UI

097b1c4

baymac force-pushed the fix/secret-token branch from e97b145 to 097b1c4 Compare May 24, 2020 14:19

Fix validToken check when X-GitLab-Token header not set

df3046f

baymac requested a review from jetersen May 28, 2020 00:26

baymac merged commit 66f422b into master May 31, 2020

baymac deleted the fix/secret-token branch May 31, 2020 12:24

dubrsl reviewed Oct 15, 2020

View reviewed changes

TBBle mentioned this pull request Jan 28, 2021

No longer works without secret token #106

Open

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

[JENKINS-62375] Fix/secret token #91

[JENKINS-62375] Fix/secret token #91

Uh oh!

Conversation

baymac commented May 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baymac commented May 21, 2020

Uh oh!

jetersen commented May 21, 2020

Uh oh!

baymac commented May 22, 2020

Uh oh!

jetersen commented May 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!