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

Use CA certs on system #23

Closed
wants to merge 1 commit into from
Closed

Use CA certs on system #23

wants to merge 1 commit into from

Conversation

jmccann
Copy link

@jmccann jmccann commented Mar 2, 2016

I ran into issues with drone-cli being able to communicate to our drone instance that is fronted by an internally signed certificate.

$ drone secure --repo org/repo --in secrets.yml --checksum
Get https://drone.test.com/api/repos/org/repo/key: x509: certificate signed by unknown authority

golang does have PRIVATE methods for getting the system root CAs. (root*.go files @ https://github.com/golang/go/tree/master/src/crypto/x509). This PR borrows that code and makes it public. I've only grabbed the code for OS X but other platforms could be adopted following the established pattern.

The best solution would be to make x509.systemRootsPool public in golang and use it directly. This seems to have been requested @ golang/go#13335

I tested this locally by updating drone-cli to use my fork of drone-go it worked.

@CLAassistant
Copy link

CLA assistant check
All committers have accepted the CLA.

@jackspirou
Copy link
Contributor

@bradrydzewski yesterday we chatted about the possibility of having drone.Client accept the http.Client in its constructor. That way we can move the syscerts repo package into the drone-cli repository within the vendor directory. That would keep the drone-go repo dependency free as a library :)

So... If that is still the direction you would like to take, can we merge this PR and then make the necessary changes to both the drone-cli and drone-go package to satisfy the above strategy? Or do you feel this PR needs to be reworked and a new PR also needs to be created for drone-cli?

@jmccann
Copy link
Author

jmccann commented Mar 15, 2016

So I'm thinking this can be closed with #24 and harness/drone-cli#30 having been merged ... right?

@jackspirou
Copy link
Contributor

@jmccann yup, I think we can close this.

@jmccann jmccann closed this Mar 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants