NOISSUE - Post-handshake aTLS#582
Conversation
04d73e6 to
da4104d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #582 +/- ##
==========================================
- Coverage 73.57% 67.13% -6.45%
==========================================
Files 96 116 +20
Lines 6123 7223 +1100
==========================================
+ Hits 4505 4849 +344
- Misses 1204 1794 +590
- Partials 414 580 +166 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if req != nil && !bytes.Equal(certMsg.Context, reqCtx) { | ||
| return nil, ErrContextMismatch | ||
| } | ||
| if len(certMsg.Entries) == 0 { |
There was a problem hiding this comment.
When the first message is Certificate (not Finished), the code parses all three messages (Certificate, CertificateVerify, Finished) on lines 242-265. However, if certMsg.Entries is empty, this block returns success without verifying the Finished MAC.
This differs from the proper empty authenticator path (lines 210-235) which correctly verifies the Finished message. An attacker could exploit this by sending Certificate(entries=[]) + CertificateVerify + Finished with arbitrary data in the Finished message.
Per the EA specification, an empty authenticator should be a Finished-only message, not a Certificate with empty entries followed by CertificateVerify and Finished.
There was a problem hiding this comment.
Thanks, this will be changed to so that a Certificate message with zero entries followed by CertificateVerify/Finished will be treated as malformed and will not be accepted as an empty authenticator.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| var verifierPolicy eaattestation.VerificationPolicy |
There was a problem hiding this comment.
When attPolicy is nil, a zero-value VerificationPolicy is created with nil interface fields. However, VerifyPayload explicitly returns ErrEvidenceVerificationMissing if evidence is present but EvidenceVerifier is nil (line 40), and ErrResultsVerificationMissing if attestation results are present but ResultsVerifier is nil (line 48). Ensure the payload is guaranteed to have no evidence and no attestation results before passing a zero-value policy, or provide default verifiers when attPolicy is nil.
There was a problem hiding this comment.
I will add a comment for this situation.
| return tls.Certificate{Certificate: [][]byte{der}, PrivateKey: priv}, leaf | ||
| } | ||
|
|
||
| func tls13Client(t *testing.T, cert tls.Certificate) *tls.Conn { |
There was a problem hiding this comment.
The tls13Client helper creates a TLS server (srv) that is never closed. While cli.Close() is called by test cleanup, the server side of the net.Pipe connection may not be properly cleaned up. Return and close both connections or defer cleanup within the helper.
There was a problem hiding this comment.
Thanks, this will be changed.
| return nil | ||
| } | ||
|
|
||
| func equalBytes(a, b []byte) bool { |
There was a problem hiding this comment.
equalBytes uses early-return comparison which leaks timing information about where mismatches occur. For cryptographic values like AIKPubHash and Binding, this could enable timing attacks. Use crypto/subtle.ConstantTimeCompare instead.
There was a problem hiding this comment.
Thanks, this will be fixed.
| return &Conn{Conn: tlsConn, Request: req, ValidationResult: res}, nil | ||
| } | ||
|
|
||
| func Server(tlsConn *tls.Conn, cfg *ServerConfig) (*Conn, error) { |
There was a problem hiding this comment.
Server requires cfg.TLSConfig != nil (line 99), but TLSConfig is not used in the function body—only cfg.Identity and cfg.BuildLeafExtensions are used. If identity is provided via cfg.Identity, TLSConfig shouldn't be mandatory.
There was a problem hiding this comment.
Thanks. This will be fixed.
| return tls.Certificate{}, err | ||
| } | ||
|
|
||
| return tls.Certificate{ |
There was a problem hiding this comment.
Setting Leaf: template uses the template *x509.Certificate which doesn't have fields like Raw, RawTBSCertificate, etc. populated. The Leaf field should contain the parsed certificate from the DER bytes for proper functionality when Go's TLS stack accesses it.
There was a problem hiding this comment.
Thanks, this will be changed.
| return nil, fmt.Errorf("failed to load auth certificates: %w", err) | ||
| } | ||
|
|
||
| tlsConfig := &tls.Config{ |
There was a problem hiding this comment.
The tls.Config lacks an explicit MinVersion, defaulting to TLS 1.0 for server connections. Since this ATLS implementation relies on TLS 1.3 keying material (exported authenticators), enforce the minimum version explicitly.
There was a problem hiding this comment.
Thanks, this will be changed.
| AttestationPolicy: atls.VerificationPolicyFromEvidenceVerifier(atls.NewEvidenceVerifier(agcfg.AttestationPolicy)), | ||
| } | ||
|
|
||
| opts = append(opts, |
There was a problem hiding this comment.
grpc.WithContextDialer is where gRPC enforces connection deadlines and cancellation. Discarding ctx here means a stalled TCP/TLS/ATLS handshake can outlive the caller's deadline and hold subchannel creation open until the underlying socket times out. The pkg/atls package currently exposes only Dial(network, address string, cfg *ClientConfig) with no context support, requiring either a context-aware variant in ATLS or an alternative approach to propagate deadlines
There was a problem hiding this comment.
Implemented. The ctx from grpc.WithContextDialer was previously dropped because pkg/atls only exposed a non-context Dial(...) API. I added context-aware dial support in conn.go and transport.go, and updated grpc.go to call atls.DialContext(ctx, ...).
| AttestationPolicy: atls.VerificationPolicyFromEvidenceVerifier(atls.NewEvidenceVerifier(agcfg.AttestationPolicy)), | ||
| } | ||
|
|
||
| transport.DialTLSContext = func(ctx context.Context, network, addr string) (net.Conn, error) { |
There was a problem hiding this comment.
Once DialTLSContext is set, http.Transport stops applying TLSHandshakeTimeout itself. The ctx parameter carries request cancellation and timeout deadlines, but discarding it prevents the ATLS connection establishment from respecting them. This can cause the connection to hang past the configured timeout or ignore request cancellation.
Additionally, atls.Dial() does not accept a context parameter, and atls.DialWithDialer() internally calls d.Dial() rather than d.DialContext(ctx). For proper timeout and cancellation propagation, the atls package needs a context-aware variant (e.g., DialWithDialerContext) that passes context through the entire TCP/TLS/EA handshake.
Same verification applies as the gRPC dialer: confirm that atls package gains DialContext support so both HTTP and gRPC clients can propagate cancellation and deadlines through the TCP/TLS/EA handshake.
| } | ||
| tlsConfig.NextProtos = []string{"h2", "http/1.1"} | ||
|
|
||
| p.started = true |
There was a problem hiding this comment.
p.started is set before listener creation completes, masking startup failures.
Setting p.started = true at line 151 before the goroutine creates the listener means Start() returns success even if listener creation fails. The caller has no indication that startup failed. Consider synchronizing listener creation before returning, or using a channel/sync mechanism to report errors.
There was a problem hiding this comment.
Implemented. Start() in proxy.go now creates the listener synchronously before setting p.started = true or returning success. This applies to plain HTTP, regular TLS, and attested TLS. The goroutine only starts Serve(listener) after listener creation succeeds, so bind/listener setup failures are now returned directly to the caller instead of being logged after a false-successful startup.
What type of PR is this?
This is a feature because it introduces post-handshake attestation with exported authenticators. The work is based on:
https://www.rfc-editor.org/rfc/rfc9261.html
https://datatracker.ietf.org/doc/html/draft-fossati-seat-expat
What does this do?
This PR replaces the legacy pkg/atls certificate-extension-based aTLS implementation with the new Exported Authenticator (EA) based aTLS transport.
The new design keeps crypto/tls as the base secure channel and performs attestation as the first post-handshake message exchange using Exported Authenticators. This aligns the cocos integration with the phase-3 EA/aTLS implementation and removes the old nonce/SNI + VerifyPeerCertificate flow.
For attested connections, the connection flow is now:
This guarantees that the first post-handshake application data on the TLS channel is the EA exchange.
Which issue(s) does this PR fix/relate to?
No issue.
Have you included tests for your changes?
Yes, tests are included.
Did you document any new/modified feature?
No, the documentation will be updated in a separate PR.
Notes