Skip to content

go: use NIX_SSL_CERT_FILE for crypto/x509#24058

Merged
copumpkin merged 1 commit intoNixOS:masterfrom
LnL7:go-cacert
Mar 26, 2017
Merged

go: use NIX_SSL_CERT_FILE for crypto/x509#24058
copumpkin merged 1 commit intoNixOS:masterfrom
LnL7:go-cacert

Conversation

@LnL7
Copy link
Copy Markdown
Member

@LnL7 LnL7 commented Mar 19, 2017

Motivation for this change

Fixes issues with crypto/x509 since SSL_CERT_FILE was renamed to NIX_SSL_CERT_FILE, eg.

$ go get -v gopkg.in/yaml/v1
Fetching https://gopkg.in/yaml/v1?go-get=1
fatal error: unexpected signal during runtime execution
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x178a91f]

runtime stack:
runtime.throw(0x149e811, 0x2a)
	/nix/store/w1990ccmglr59q6aqj60gf6fz63619nq-go-1.8/share/go/src/runtime/panic.go:596 +0x95
runtime.sigpanic()
	/nix/store/w1990ccmglr59q6aqj60gf6fz63619nq-go-1.8/share/go/src/runtime/signal_unix.go:274 +0x2db

/cc @zimbatm

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@LnL7 LnL7 added 6.topic: darwin Running or building packages on Darwin 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. labels Mar 19, 2017
@mention-bot
Copy link
Copy Markdown

mention-bot commented Mar 19, 2017

@LnL7, thanks for your PR! By analyzing the history of the files in this pull request, we identified @avnik, @globin and @edolstra to be potential reviewers.

@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Mar 19, 2017

Is this also relevant for nixos (regarding backport)?

@LnL7
Copy link
Copy Markdown
Member Author

LnL7 commented Mar 19, 2017

I would say yes. A single user install only sets NIX_SSL_CERT_FILE so go would not use the cert bundle from nixpkgs unlike other tools like curl, git, ...

zimbatm
zimbatm approved these changes Mar 19, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@zimbatm zimbatm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This will fix the segfaults on Darwin but is really a workaround.

As a general rule, programs should read from the host's CA. Otherwise it's not possible for users to set their own certificate chains for internal networks.

@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Mar 19, 2017 •
edited
Loading

In that case I would rather have this patch, if not necessary (on linux).

@LnL7
Copy link
Copy Markdown
Member Author

LnL7 commented Mar 19, 2017

@zimbatm There's no file with the cert bundle on darwin so it's not possible to use the host's CA like on other systems. I don't know if this is an issue with cgo or the loadSystemRoots implementation.

@zimbatm
Copy link
Copy Markdown
Member

zimbatm commented Mar 20, 2017 •
edited
Loading

This seems to be related to our issue: golang/go#17972 . Basically Go's interface to the native Mac TLS/CA library is not very stable.

I agree that the patch could be applied only on Darwin as it's a workaround just for that platform. Actually it could also come in handy in a nix sandbox fetcher environment

@copumpkin copumpkin mentioned this pull request Mar 20, 2017
Closed
copumpkin
copumpkin reviewed Mar 20, 2017
View reviewed changes
pkgs/development/compilers/go/1.7.nix
];

SSL_CERT_FILE = "${cacert}/etc/ssl/certs/ca-bundle.crt";
NIX_SSL_CERT_FILE = "${cacert}/etc/ssl/certs/ca-bundle.crt";
Copy link
Copy Markdown
Member

@copumpkin copumpkin Mar 20, 2017

Choose a reason for hiding this comment

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

Why does this need to be set in the build env?

Copy link
Copy Markdown
Member

@edolstra edolstra Mar 20, 2017

Choose a reason for hiding this comment

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

Yeah, it's unclear to me why Go would need a certificate bundle at build time. Is it going to download something via https?

Copy link
Copy Markdown
Member

@copumpkin copumpkin Mar 20, 2017

Choose a reason for hiding this comment

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

I hope not! Perhaps it does some sort of sanity check during tests? Seems weird though...

Copy link
Copy Markdown
Member Author

@LnL7 LnL7 Mar 20, 2017

Choose a reason for hiding this comment

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

Yes, it's for the tests

@copumpkin
Copy link
Copy Markdown
Member

copumpkin commented Mar 20, 2017

So it seems like we should open a separate issue about the cgo interface to the mac native certificate chain, since that should work as a fallback, even if we merge this.

@copumpkin copumpkin mentioned this pull request Mar 20, 2017
Closed
@copumpkin copumpkin merged commit 826ae5f into NixOS:master Mar 26, 2017
@Gabriella439
Copy link
Copy Markdown
Contributor

Gabriella439 commented May 4, 2017

I ran into this issue today. If you have an older Nix installation (like I did) which defines SSL_CERT_FILE instead of NIX_SSL_CERT_FILE you need to update your profile to add:

export NIX_SSL_CERT_FILE="${SSL_CERT_FILE}"

... and then the segmentation fault goes away

@LnL7 LnL7 deleted the go-cacert branch September 21, 2017 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zimbatm zimbatm zimbatm approved these changes

+2 more reviewers

@copumpkin copumpkin copumpkin left review comments

@edolstra edolstra edolstra left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: darwin Running or building packages on Darwin 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@LnL7 @mention-bot @Mic92 @zimbatm @copumpkin @Gabriella439 @edolstra