Skip to content

Conversation

mat1e
Copy link
Member

@mat1e mat1e commented Aug 17, 2022

Link to #150

This PR change Sonarqube token to Jenkins credentials.

The environnement variable doesn't change but must refers to a valid credentailsId.
The credentials type is a StringCredentials and it must be set in the context of the Job and contains the token to connect to Sonarqube.

@mat1e mat1e changed the title Feature/credentials for sonarqube change token to credentials for sonarqube Aug 18, 2022
@asimell
Copy link
Contributor

asimell commented Aug 30, 2022

SONAR_AUTH_TOKEN is an automatically exposed variable when using something like this

withSonarQubeEnv("my-sonarqube") {
    influxDbPublisher selectedTarget: my-target
}

it finds the token and uses that. I'd assume this PR allows me to not have to use the withSonarQubeEnv closure, but is there any other benefit for this refactor?

@mat1e
Copy link
Member Author

mat1e commented Aug 30, 2022

The benefit is to use credentials wich is more secure than use directly a token setted as environment variable. Now SONAR_AUTH_TOKEN must represent the credentialsId wich contains the token.
But you're right, maybe there is a better way, I did it like that to not change behaviour too much...

@asimell
Copy link
Contributor

asimell commented Sep 6, 2022

I feel like this works a little unintuitively. Consider the following scenario

stage("Sonarqube") {
    environment {
        SONAR_AUTH_TOKEN = "credentials-id" // 1
    }
    steps {
        withSonarQubeEnv("myEnv") {
            scannerHome = tool "mySonar"
            sh "${scannerHome}/bin/sonar-scanner"
            influxDbPublisher selectedTarget: "My Target" // 2
        }
        influxDbPublisher selectedTarget: "My Target" // 3
    }
}

In 1, we're giving a variable called SONAR_AUTH_TOKEN a value something other than the actual token.
In 2, the SONAR_AUTH_TOKEN given in the environment closure of the stage is overwritten with the actual token exposed from withSonarQubeEnv meaning that the write will fail.
3 will work as expected since we have set the environment variable in the environment closure.

This being said, I do believe that using the credentials ID is better than having to wrap the plugin in an unnecessary withSonarQubeEnv as is the current behaviour. However, I just find it incredibly confusing to use a variable differently than is actually intended by the tool. I feel like this would be much clearer if instead of SONAR_AUTH_TOKEN environment variable, some other variable was used meaning that there would essentially come a third option to the existing 2 strategies now:

  1. Look for env variable with credentials id. If a match is found, use that. I'm not 100% if there's a "correct" way to put this in a Jenkins pipeline.
  2. If credentials id is not found, try SONAR_AUTH_TOKEN to look for the token (current behaviour).
  3. If no token is found, try to check unauthorized potentially giving 401 (current behaviour).

Apologies for nitpicking on this change, which I think is an improvement, but almost every single time there's a PR targeting SonarQubeGenerator I've had to quickly release a hotfix, because the PR broke something essential.

@mat1e
Copy link
Member Author

mat1e commented Sep 6, 2022

There is somes improvement to do to this PR. I used the already defined variable SONAR_AUTH_TOKEN to not modify the behavior too much. But I continu to think it is not a good practice to use an environment variable for an authentication token.

The best way is to use withCredentials. Credentials are encrypted on the diskspace with the key of the Jenkins instance and it is more secure to use.

I will provide an other behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants