WIP: X509 integration
What does this MR do?
Adds the ability to configure and use x509 certificate and key pairs for TLS for connecting to gitlab servers which require authentication
Why was this MR needed?
When making requests with gitlab-ci-multi-runner to gitlab servers which require certificate authentication it is presently impossible to do so. This allows for runner to utilize a certificate/key pair to make the connection.
Are there points in the code the reviewer needs to double check?
I am not 100% clear on how this project handles documentation. I would ask the reviewer to take look and suggest how they would prefer documentation to be included. (ie which files to add too or if new files should be made etc...)
Does this MR meet the acceptance criteria?
-
Documentation created/updated - Tests
-
Added for this feature/bug -
All builds are passing
-
-
Branch has no merge conflicts with master(if you do - rebase it please)
What are the relevant issue numbers? none
additional notes
Some tests seem to not work despite seemingly being unrelated to my code. Specifically there are tests for the virtual box executor which pass the initial acceptance criteria but then fail because of a Minute time out fail switching from minute to hour gets these tests to pass but I am not sure why they are failing in the first place.
the docker executor test seems to time out. This started to happen after a rebase to 1-6 stable removed a dependency
Merge request reports
Activity
Added 23 commits:
- 9575f427...2d7d2fc0 - 4 commits from branch
gitlab-org:master - 80305741 - Initial X509 parsing attempt
- 550d08e4 - fmt formatting, fixed two fmt errors, got another
- eed34fd0 - Fix line 101 tls config error
- 9b3bc746 - implimented tls key and pem to core functionality
- 0598e738 - fixed issue where certificate variables were not properly set for artifacts
- b9a665fd - add client test and refacotred client.go
- e06284c1 - made changes to docker dependencies to avoid test breaks
- 4a70e5bb - cleaned up some spacing issues
- 09b7b7cc - more spacing fixes
- 481ff40a - Initial X509 parsing attempt
- 80674ffb - implimented tls key and pem to core functionality
- 4e8509df - fixed issue where certificate variables were not properly set for artifacts
- c7b04d6e - made changes to docker dependencies to avoid test breaks
- 142d53b9 - cleaned up some spacing issues
- 07522182 - more spacing fixes
- a460ee74 - converted a file not found error to a warning concerning the key and pem files
- ac71380b - fixed regressions made from rebasing to 1-6 ran go fmt
- 4a9ec027 - capped tls in config descriptions
- 7f98c73b - added documentation for x509 tls feature
Toggle commit list- 9575f427...2d7d2fc0 - 4 commits from branch
Thanks for this contribution @Saltychipmunk - I can see the use case for this, and it's something I'd love us to support. I've left some comments on the work so far, hopefully they help!
Added feature proposal label
@Saltychipmunk I've also discovered two prior merge requests on the subject: !86 (closed) and !157 (merged)
The latter's approach to putting the key and certificate into the GetBuildResponse struct is preferable to the extra return values in doJSON in this MR; there's also a few comments from @ayufan that would apply to this MR as well. In particular:
Milestone changed to %v1.7
Ahh, I think I understand what you want , That should be possible. the methods that user is using already exist in client.go. I can probably move them over with no issue. Ill impliment and see how it goes
Edited by username-removed-676625Ok I have tested this , from what I can tell this wont work if the user does not set the tls-client-cert-file and tls-client-key-file values in the config.toml.
But it will work if they do. So this leads back to an earlier question.. should there be a default value set for these files or should we leave this up to the user to set them in the config?
Ok, I have made a pretty considerable change and will need your preference before proceeding any further. As of right now I have introduced code into gitlab.go which will set a default file path to the key and pem file (following the format seen in client.go newClient) this will allow a user to not need to specify a custom directory path to the key and pem and use a default path and file name similar to the how the CA file is given a default.
However, this does mean the code will always look for a key and pem file simply because it will assign a default path if a custom one is not given.
So we need to decide do we want a default path for key and pm or do we want the user to have to deliberately specify the file paths during registration etc
@Saltychipmunk could you enable shared runners for your fork of
gitlab-ci-multi-runner? That will allow the tests to run for this MR.Just navigate to https://gitlab.com/Saltychipmunk/gitlab-ci-multi-runner/runners and click "Enable shared runners".
Added 17 commits:
- c8fed7b9...cbf65211 - 15 commits from branch
gitlab-org:master - 66b42176 - Initial X509 parsing attempt
- 5c9a46a2 - Added TLS Client auth
- c8fed7b9...cbf65211 - 15 commits from branch
Added 29 commits:
- c8b29756...e3ba61cc - 26 commits from branch
gitlab-org:master - cf833604 - Initial X509 parsing attempt
- f6c308e5 - Added TLS Client auth
- 956d8b9e - fix main.go import
Toggle commit list- c8b29756...e3ba61cc - 26 commits from branch
Thanks @Saltychipmunk. I've made a few more comments.
@tmaczukin what do you think to getting this in for v1.8 if possible?
Added 1 commit:
- e2838f2a - various fixes and changes for the commit smush
Added 27 commits:
- 0a07c6f0...0856546e - 26 commits from branch
gitlab-org:master - 9e9348fa - Merge branch 'master' of https://gitlab.com/gitlab-org/gitlab-ci-multi-runner into x509_integration
- 0a07c6f0...0856546e - 26 commits from branch
Hello, @Saltychipmunk do you know when you will be able to make the last changes? Because we need this kind of auth in our organization and I wasn't able to build your branch successfully. Just to have a vague ETA. Thank you
I can look at it when i have some time. What part of the build is broken? I ask because even when i was working from a stable branch there were tests that never worked for me which would cause the build tests to fail.
The tests that failed usually were the executor tests.. or the golinter would go nutty , however i could still compile and install the runner despite this
Also keep in mind that this is now built of off master branch on gitlab... and while i do rebase from time to time.. master is technically not the stable version of the runner so it could very will be a case of it being something out of my control.
I dont want to touch files that i should not be touching if i can help it.
if the issue happens to be the golinter or fmt .. i would suggest temporarily commenting them out of the makefile
Edited by username-removed-676625-
Hi Brian, I forked your branch and tried to build it on gitlab, the unit tests and the linting both failed, even though I was on the same commit that passed those steps on your repo. I tried to build it on my machine too but with no success because some deps could not be found.
I don't understand the state your MR is in. Is the gitlab team waiting for you to make some change or are you waiting for them to approve it? Because if so, we could @ them.
Ok so full update. It is not fully clear if/ when the main decision makers @ git lab will want to integrate thisfunctionality into the core multi-runner. due tothis ambiguity I have stopped developing on the x509 branch. This effectivly means i have stopped rebasing it as master updates.. so there is a very good chance that it has out of date code etc.
There was a point where it was almost merge ready but it seems for the time being the gitlab leadership have other development priorites.. I have made the changes they requested.. a whole bunch of the open code fix requests are out of date.. they ... just ... stopped looking at it.
I have created a separate branch *1-7-stable-x509-integration https://gitlab.com/Saltychipmunk/gitlab-ci-multi-runner/commits/1-7-stable-x509-integration
- which is my changes applied to the 1-7 stable release branch
As I have said before the linter will fail .. because of code i have no control over. thus i disabled it in the make file locally as for the other unit test failing... if the test is an executor test.. the reason it isfailing may very well be because the timeout rules gitlab uses for their tests tend to be strict and if you dont have a perfect machine .. the tests might just timeout. and boom fail.
As for the deps.. did you run "make deps"? this should install the dependencies
I built this on an ubuntu machine by following the directions given in the "develpment" doc.
-
Hello Brian, thanks for your answer and your time. I understand what you are saying. Thanks for the branch you created. I'll try to build against it. I think I did
make deps, but since it works now, I may not have tried to install the deps this way last time. I'll try to build, but it should work now. Thanks for your help and your fast answers! It's really nice to have people so responsive and helpful :) mentioned in issue #2277 (closed)
Closed in favour of !157 (merged)
Thanks for your work on this @Saltychipmunk but it seems we've just merged another MR implementing it.
