Skip to content

x/crypto/acme/autocert: revokePending with nil client #25581

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.

Sign up for GitHub

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

Jump to bottom
Closed
immesys opened this issue May 26, 2018 · 6 comments
Closed

x/crypto/acme/autocert: revokePending with nil client #25581

immesys opened this issue May 26, 2018 · 6 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@immesys
Copy link

immesys commented May 26, 2018

What version of Go are you using (go version)?

1.10.1
x/crypto commit: 159ae71

Does this issue reproduce with the latest release?

This is one commit away from master, nothing relevant looks changed

What operating system and processor architecture are you using (go env)?

amd64 linux

What did you do?

I have a web service that uses autocert. It was old code that used to work but stopped working since the whole TLS-SNI-deprecation thing, so I had a few runs where I tried to create a certificate for a new domain and it failed. I modified the code to tie in HTTPHandler so that the HTTP-01 challenge would work and re-ran it.

The code is basically:

m := autocert.Manager{
	Prompt:     autocert.AcceptTOS,
	Cache:      keys.NewEtcdCache(etcdConn), //old code that worked before
	HostPolicy: autocert.HostWhitelist(autocertHostname),
	Email:      email,
}
//replace the handler
httpHandler = m.HTTPHandler(httpHandler)
...
//start http server with httpHandler
//make a tls config that has m.GetCertificate in it and start https server with httpHandler

It crashes with the following when I access it over https

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x9853c6]

1: running [Created by autocert.(*Manager).revokePending @ autocert.go:555]
    acme         acme.go:390                   (*Client).RevokeAuthorization(*Client(0x0), Context(0xdca980), string(0xc42025e2a0, len=91), 0x0, 0x0)
    autocert     autocert.go:549               (*Manager).revokePending.func1(0xdca980, #1)
1: select [Created by clientv3.(*watchGrpcStream).newWatchClient @ watch.go:781]
    transport    transport.go:142              (*recvBufferReader).read(*recvBufferReader(#55), []byte(#33 len=5 cap=5), 0xffffffffffffffff, 0x100, 0x3)
    transport    transport.go:131              (*recvBufferReader).Read(*recvBufferReader(#55), []byte(#33 len=5 cap=5), 0x0, 0x0, 0x4f1f30)
    transport    transport.go:369              (*transportReader).Read(*transportReader(#112), []byte(#33 len=5 cap=5), 0x40ee5d, #90, 0x793858)
    io           io.go:309                     ReadAtLeast(Reader(0xdc0b80), []byte(#33 len=5 cap=5), 5, #23, #70, 0x5)
    io           io.go:327                     ReadFull(Reader(0xdc0b80), []byte(#33 len=5 cap=5), #91, 0x4100dd, 0x412ace)
    transport    transport.go:353              (*Stream).Read(*Stream(#70), []byte(#33 len=5 cap=5), #92, 0x412c39, 0x4271fe)
    grpc         rpc_util.go:452               (*parser).recvMsg(*parser(#32), 2147483647, 0x20, 0x4271fe, 0xd51e08, 0x1, #93, 0x4573f4)
    grpc         rpc_util.go:561               recv(*parser(#32), baseCodec(#137), *Stream(#70), Decompressor(0x0), interface{}(0xccbfa0), 2147483647, *InPayload(0x0), ...)
    grpc         stream.go:529                 (*csAttempt).recvMsg(*csAttempt(#62), interface{}(0xccbfa0), 0x0, 0x0)
    grpc         stream.go:395                 (*clientStream).RecvMsg(*clientStream(#46), interface{}(0xccbfa0), 0x43ad8b, 0x0)
    etcdserverpb rpc.pb.go:3517                (*watchWatchClient).Recv(*watchWatchClient(#108), 0x0, 0x0, 0x0)
    clientv3     watch.go:626                  (*watchGrpcStream).serveWatchClient(#64, 0xdcd660, #108)
2: select [Created by transport.newHTTP2Client @ http2_client.go:296]
    transport    controlbuf.go:289             (*controlBuffer).get(*, 0x1, 0, 0, 0, 0)
    transport    controlbuf.go:374             (*loopyWriter).run(*)
    transport    http2_client.go:298           newHTTP2Client.func3(*)
1: runnable [Created by http.(*Server).Serve @ server.go:2795]
    sync         sema.go:56                    runtime_Semacquire(*uint32(#80))
    sync         rwmutex.go:50                 (*RWMutex).RLock(*RWMutex(#79))
    autocert     autocert.go:311               (*Manager).cert(*Manager(#44), Context(0xdca9c0), string(#114, len=15), 0x0, 0x0, 0x0)
    autocert     autocert.go:222               (*Manager).GetCertificate(*Manager(#44), *ClientHelloInfo(#45), 0x0, 0x0, 0x0)
    autocert     serveplotter.go:197           (*Manager).GetCertificate-fm(*Config(#45), 0x4121d8, 0xb0, 0xc9f320)
    tls          common.go:712                 (*Config).getCertificate(*Config(#113), 842352600976, 0x2, #59, 0x2)
    tls          handshake_server.go:216       (*serverHandshakeState).readClientHello(*serverHandshakeState(#88), #87, #89, 0x411485)
    tls          handshake_server.go:48        (*Conn).serverHandshake(*Conn(#25), 0xd51e28, #26)
    tls          conn.go:1331                  (*Conn).Handshake(*Conn(#25), 0x0, 0x0)
    http         server.go:1742                (*conn).serve(#69, 0xdcaa00, #18)
1: IO wait [Created by transport.newHTTP2Client @ http2_client.go:265]

< more that I don't think is relevant >

What did you expect to see?

I expected a certificate to get issued and life to be easy.

What did you see instead?

The above panic.

If I made some mistake I would also expect a normal error, not a nil pointer deep inside autocert. My usage is so simple though, so I don't think this is my fault.
@gopherbot gopherbot added this to the Unreleased milestone May 26, 2018
@immesys
Copy link
Author

immesys commented May 26, 2018

@costela I think this piece of code was yours. Any ideas how I can help debug?

@immesys
Copy link
Author

immesys commented May 26, 2018

FYI, if I pass the client from the parameters to verify() through to revokePending rather than using m.Client then everything works.

immesys added a commit to immesys/crypto that referenced this issue May 26, 2018
@immesys
acme/autocert: use client from verify

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
3734d7b 
revokePending assumes the Client field of the manager has been
populated, but this might not be true. Use the client passed
to the parent verify() function instead.

Fixes golang/go#25581
@immesys immesys mentioned this issue May 26, 2018
Closed
@immesys
Copy link
Author

immesys commented May 26, 2018

I opened a PR that fixes it, but I didn't dig into why the assumption that m.Client is populated did not hold up.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/114817 mentions this issue: acme/autocert: improve authorizations cleanup

@costela
Copy link
Contributor

costela commented May 26, 2018

@immesys yup, my bad.
@x1ddos seems to already have stepped in to clean it up! :)

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/114801 mentions this issue: acme/autocert: use client from verify

@golang golang locked and limited conversation to collaborators May 27, 2019
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
acme/autocert: improve authorizations cleanup
f597cbe 
Fixes a bug introduced in golang.org/cl/100078 where incorrect
ACME client was used, causing nil pointer dereference.

The change also improves related tests,
removing code paths diverging in testing.

Fixes golang/go#25581

Change-Id: I8c5531fcc5814a5a64f14911c0ad86c476a76d2f
Reviewed-on: https://go-review.googlesource.com/114817
Reviewed-by: Filippo Valsorda <filippo@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
acme/autocert: improve authorizations cleanup
fa94cea 
Fixes a bug introduced in golang.org/cl/100078 where incorrect
ACME client was used, causing nil pointer dereference.

The change also improves related tests,
removing code paths diverging in testing.

Fixes golang/go#25581

Change-Id: I8c5531fcc5814a5a64f14911c0ad86c476a76d2f
Reviewed-on: https://go-review.googlesource.com/114817
Reviewed-by: Filippo Valsorda <filippo@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
acme/autocert: improve authorizations cleanup
31f326c 
Fixes a bug introduced in golang.org/cl/100078 where incorrect
ACME client was used, causing nil pointer dereference.

The change also improves related tests,
removing code paths diverging in testing.

Fixes golang/go#25581

Change-Id: I8c5531fcc5814a5a64f14911c0ad86c476a76d2f
Reviewed-on: https://go-review.googlesource.com/114817
Reviewed-by: Filippo Valsorda <filippo@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
acme/autocert: improve authorizations cleanup
72f5d83 
Fixes a bug introduced in golang.org/cl/100078 where incorrect
ACME client was used, causing nil pointer dereference.

The change also improves related tests,
removing code paths diverging in testing.

Fixes golang/go#25581

Change-Id: I8c5531fcc5814a5a64f14911c0ad86c476a76d2f
Reviewed-on: https://go-review.googlesource.com/114817
Reviewed-by: Filippo Valsorda <filippo@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
acme/autocert: improve authorizations cleanup
0ec932c 
Fixes a bug introduced in golang.org/cl/100078 where incorrect
ACME client was used, causing nil pointer dereference.

The change also improves related tests,
removing code paths diverging in testing.

Fixes golang/go#25581

Change-Id: I8c5531fcc5814a5a64f14911c0ad86c476a76d2f
Reviewed-on: https://go-review.googlesource.com/114817
Reviewed-by: Filippo Valsorda <filippo@golang.org>
desdeel2d0m added a commit to desdeel2d0m/crypto that referenced this issue Jul 1, 2024
@desdeel2d0m
acme/autocert: improve authorizations cleanup
728914c 
Fixes a bug introduced in golang.org/cl/100078 where incorrect
ACME client was used, causing nil pointer dereference.

The change also improves related tests,
removing code paths diverging in testing.

Fixes golang/go#25581

Change-Id: I8c5531fcc5814a5a64f14911c0ad86c476a76d2f
Reviewed-on: https://go-review.googlesource.com/114817
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@costela @immesys @gopherbot