Skip to content

openssl: Add variant to use system certificates#6396

Merged
alalazo merged 1 commit intospack:developfrom
michaelkuhn:openssl-trust-system
Mar 26, 2018
Merged

openssl: Add variant to use system certificates#6396
alalazo merged 1 commit intospack:developfrom
michaelkuhn:openssl-trust-system

Conversation

@michaelkuhn
Copy link
Copy Markdown
Member

@michaelkuhn michaelkuhn commented Nov 21, 2017

Spack's openssl package is currently a bit useless because it can not verify any certificates. This PR adds a system-certs variant that symlinks the system certificates into the package.

@michaelkuhn michaelkuhn force-pushed the openssl-trust-system branch from 182d6ea to e775dcf Compare March 22, 2018 09:48
@michaelkuhn
Copy link
Copy Markdown
Member Author

michaelkuhn commented Mar 22, 2018

@tgamblin @adamjstewart Any opinions on this? It fixes problems like this when using Spack's openssl:

$ wget https://github.com/spack/spack/releases/download/v0.11.2/spack-0.11.2.tar.gz
ERROR: cannot verify github.com's certificate, issued by 'CN=DigiCert SHA2 Extended Validation Server CA,OU=www.digicert.com,O=DigiCert Inc,C=US':
  Unable to locally verify the issuer's authority.
To connect to github.com insecurely, use `--no-check-certificate'.

tgamblin
tgamblin requested changes Mar 22, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@tgamblin tgamblin left a comment

Choose a reason for hiding this comment

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

This is pretty awesome. I definitely think it should be the default.

Can you add support for macOS as well? There, you have to use the security command to dump the system certs, but I don't believe it is much more code than what you already have.

@michaelkuhn
Copy link
Copy Markdown
Member Author

michaelkuhn commented Mar 22, 2018

Can you add support for macOS as well? There, you have to use the security command to dump the system certs, but I don't believe it is much more code than what you already have.

Sorry, I do not have access to a macOS system. It probably makes more sense if someone else takes care of that part. Homebrew has a formula that can probably be used as a basis (https://github.com/Homebrew/homebrew-core/blob/master/Formula/openssl.rb).

tgamblin
tgamblin reviewed Mar 22, 2018
View reviewed changes
var/spack/repos/builtin/packages/openssl/package.py Outdated
version('1.0.1r', '1abd905e079542ccae948af37e393d28')
version('1.0.1h', '8d6d684a9430d5cc98a62a5d8fbda8cf')

variant('system-certs', default=True, description='Use system certificates')
Copy link
Copy Markdown
Member

@tgamblin tgamblin Mar 22, 2018

Choose a reason for hiding this comment

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

Can you remove the - from the variant name here? Technically it is allowed but we decided to prefer not to use it. use systemcerts

Copy link
Copy Markdown
Member Author

@michaelkuhn michaelkuhn Mar 22, 2018

Choose a reason for hiding this comment

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

Done. :-)

@tgamblin
Copy link
Copy Markdown
Member

tgamblin commented Mar 22, 2018

It probably makes more sense if someone else takes care of that part.

Ok -- why don't we go ahead and get this done for Linux first, then, and someone can add Mac OS in a later PR.

@michaelkuhn michaelkuhn force-pushed the openssl-trust-system branch from e775dcf to 9d576e0 Compare March 22, 2018 10:47
alalazo
alalazo approved these changes Mar 23, 2018
View reviewed changes
var/spack/repos/builtin/packages/openssl/package.py Outdated

pkg_dir = join_path(self.prefix, 'etc', 'openssl')

for dir in system_dirs:
Copy link
Copy Markdown
Member

@alalazo alalazo Mar 23, 2018

Choose a reason for hiding this comment

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

Minor note: can we use a different name, like directory? Asking just because dir is a built-in function in python.

Copy link
Copy Markdown
Member Author

@michaelkuhn michaelkuhn Mar 26, 2018

Choose a reason for hiding this comment

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

I changed it to directory as you proposed.

@michaelkuhn michaelkuhn force-pushed the openssl-trust-system branch from 9d576e0 to a960ce6 Compare March 26, 2018 08:46
@alalazo alalazo merged commit 1aed760 into spack:develop Mar 26, 2018
ch741 pushed a commit to ch741/spack that referenced this pull request Apr 20, 2018
@michaelkuhn @ChrisHad
openssl: Add variant to use system certificates (spack#6396)
6314da8
@michaelkuhn michaelkuhn mentioned this pull request Jul 17, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tgamblin tgamblin tgamblin approved these changes

@alalazo alalazo alalazo approved these changes

@adamjstewart adamjstewart adamjstewart approved these changes

+1 more reviewer

@healther healther healther approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

variants

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@michaelkuhn @tgamblin @healther @alalazo @adamjstewart