move go-libp2p-webtransport to p2p/transport/webtransport #1737
Conversation
To make this actually secure, we still need to verify the certificate hashes.
With the most recent webtransport-go, it's not necessary to block any more.
marten-seemann
force-pushed
the
merge-webtransport
branch
from
fab7475
to
04a1fe4
Compare
There was a problem hiding this comment.
Some small comments/questions.
Nothing blocking if we agree we can change the multiaddr quic/webtransport part later, but if we are committing to this right now I think we should take a bit more. Not saying we need to change it later, just that we agree we could change it.
| type certManager struct { | ||
| clock clock.Clock | ||
| ctx context.Context | ||
| ctxCancel context.CancelFunc |
There was a problem hiding this comment.
I believe this is an anti-pattern. https://go.dev/blog/context-and-structs
There was a problem hiding this comment.
Hmm, this is also the pattern we've been using all across go-libp2p, in every service that spawns Go routines. It's super convenient since you can call the CancelFunc multiple times (which you can't do if you do the naive implementation with a closeChan chan struct{}, as you can only close a channel once).
There was a problem hiding this comment.
Yeah I know we do it a lot. Just worth pointing out that I don’t think it’s idiomatic go, and maybe we should stop adding new code like this.
Not a blocking change of course
| if err != nil { | ||
| return err | ||
| } | ||
| // TODO: is this the best (and complete?) way to identify RSA certificates? |
There was a problem hiding this comment.
I think anything with the pkcs-1 oid should probably be checked here (You aren't matching oidSignatureSHA512WithRSA for example) (https://www.rfc-editor.org/rfc/rfc3279#section-2.3.1)
There was a problem hiding this comment.
I'm not sure how to check for this. We only have access to the parsed certificate, where the signature algorithm is encoded as a x509.SignatureAlgorithm. I'd assume that those other algorithms would show up as x509.UnknownSignatureAlgorithm. Maybe it's sufficient to reject that one?
|
I left a comment instead of an approval. I think another blocker is the potential stalling of the http server. If that's addressed as well as the other comment ( |
marten-seemann
force-pushed
the
merge-webtransport
branch
from
04a1fe4
to
cd3b909
Compare
|
@MarcoPolo Thank you for the comprehensive review! I've addressed all your comments and recreated this PR.
See my comment here: #1737 (comment). |
marten-seemann
force-pushed
the
merge-webtransport
branch
from
cd3b909
to
73b3d56
Compare
Part of #1717.