Drop hardcoded 3, 4 major version literals in switch statements#7
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates OpenSSL major-version dispatch logic to treat any non-1.x major as “modern” (3+), removing hardcoded case 3, 4 branches and associated unsupported-version panics to improve forward compatibility.
Changes:
- Convert
switch major()dispatch fromcase 3, 4todefaultacross multiple primitives so OpenSSL 5+ naturally follows the 3+ code paths. - Remove
checkMajorVersion(3, 4)assertions inside 3+ helper functions and remove unreachablepanic(errUnsupportedVersion())branches. - Adjust a couple of version checks from
switch major()toif major() != 1for the “fetch for performance” paths.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tls1prf.go | Routes non-1.x majors to the EVP_KDF (3+) TLS1-PRF implementation; removes 3/4-only guards. |
| rsa.go | Treats non-1.x majors as “modern” RSA key generation/import paths; removes unsupported-version panics. |
| pbkdf2.go | Routes non-1.x majors to EVP_KDF PBKDF2 fetch/derive paths; removes 3/4-only guards. |
| hmac.go | Treats non-1.x majors as EVP_MAC(HMAC) path; removes unsupported-version panics. |
| hkdf.go | Routes non-1.x majors to EVP_KDF HKDF/TLS13-KDF paths; removes 3/4-only guards. |
| evp.go | Treats non-1.x majors as 3+ for EVP_MD/EVP_PKEY generation/provider logic and OAEP label handling. |
| ed25519.go | Treats non-1.x majors as EVP_SIGNATURE-based Ed25519 support probing. |
| ecdsa.go | Routes non-1.x majors to the param-builder (3+) ECDSA key path; removes 3/4-only guards. |
| ecdh.go | Routes non-1.x majors to the param-builder (3+) ECDH key path; removes unsupported-version panics. |
| dsa.go | Routes non-1.x majors to the param-builder (3+) DSA key path; removes 3/4-only guards. |
| cipher.go | Treats non-1.x majors as 3+ for the “fetch once for performance” cipher caching path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Benchmark Results✅ ❌
|
michelle-clayton-work
changed the title
|
@qmuntal do you have a strategy in mind to tackle this switch getOSSLDigetsContext? It's a little more complex so I wasn't sure if it was as easy to just remove it |
We should mark the hash objects as not marshalable when not using a known version. |
qmuntal
left a comment
qmuntal
left a comment
There was a problem hiding this comment.
checkMajorVersion shouldn't be used anymore, no? Can it be removed?
Per ADR 0018, OpenSSL 3+ guarantees ABI/API compatibility within a major
version, and the osslsetup startup check rejects untested majors (with a
GODEBUG=ms_opensslallowuntested=1 escape hatch). Code that runs on
OpenSSL 3+ should not need to enumerate concrete majors; `case 3, 4:`
was forcing every new major to touch every file.
Convert `switch major() { case 1: ...; case 3, 4: ...; default:
panic(errUnsupportedVersion()) }` patterns to `case 1: ...; default:
...`. For single-case `switch major() { case 3, 4: ... }` patterns that
no-op on 1, switch to `if major() != 1`.
Also remove `checkMajorVersion`: every call sat inside a function only
reached from the matching arm of the outer dispatch, so the assertion
was always redundant. Update versionguardcheck to drop it from the
gated-call set.
In getOSSLDigetsContext, replace `panic(errUnsupportedVersion())` in
the default arm with `return nil`. Callers already surface nil as
errHashStateInvalid; degrading to an error is safer than crashing when
the escape hatch surfaces a major whose EVP_MD_CTX layout we don't
know.
No behavior change for the currently tested majors. Future OpenSSL 5+
will exercise the 3+ code paths automatically once whitelisted in
osslsetup/init.go testedMajors.
Hash.MarshalBinary/UnmarshalBinary parses the running EVP_MD_CTX state by overlaying a Go struct whose layout matches OpenSSL's evp_md_ctx_st for the current major. When the loaded major is one we have not tested (only reachable behind GODEBUG=ms_opensslallowuntested=1), that overlay would read unknown memory. Per qmuntal's review on #7, mark such hashes as not marshallable so MarshalBinary/UnmarshalBinary short-circuit with the existing errMarshallUnsupported{} (which unwraps to errors.ErrUnsupported) instead of risking a wrong-layout access: * osslsetup: add IsTestedMajor(m int) bool, exposing the existing testedMajors gate that openLibrary uses at startup. * openssl: add knownMajor() wrapper for the same check. * loadHash: AND the per-provider marshallable assignment with knownMajor() so untested-major hashes report not marshallable. * getOSSLDigetsContext: update the default-arm comment now that the upstream gate makes the path effectively unreachable; the nil return becomes defense in depth.
michelle-clayton-work
force-pushed
the
dev/mclayton/case-default-cleanup
branch
from
e9cecb8 to
031ad9f
Compare
Let me know if this makes sense -- added some testedMajors wrapper, then gated hash.marshallable on it in loadHash and MarshalBinary/UnmarshalBinary now short-circuit with the existing errMarshallUnsupported{} instead of touching unknown EVP_MD_CTX memory |
Need to clean up leftover switch statements that hardcoded ossl 3 and 4 to fully support forward compatibility.
Convert:
switch major() {
case 1: ... legacy ...
case 3, 4: ... modern ...
default: panic(errUnsupportedVersion())
}
to:
switch major() {
case 1: ... legacy ...
default: ... modern ...
}
No behavior change for the currently tested majors. Future OpenSSL 5+ will exercise the 3+ code paths automatically once added to testedMajors in osslsetup/init.go.