refactor: Enabled ODCA verification for rpc-go#1329
Draft
ShradhaGupta31 wants to merge 3 commits into
Draft
Conversation
ShradhaGupta31
force-pushed
the
odca-verification
branch
from
bf82aa4 to
ff2ce67
Compare
- Added all the ODCA check as per Intel ODCA standard documents
There was a problem hiding this comment.
Pull request overview
This PR updates rpc-go’s local AMT TLS handling to perform stricter ODCA-related certificate verification during activation (especially in pre-provisioning mode), and threads additional context (UPID) into the TLS verifier.
Changes:
- Extends
certs.GetTLSConfig/VerifyCertificatesto support ODCA verification with optional UPID binding and CRL checks. - Updates activation/deactivation/diagnostics call sites for the new TLS config signature and adds more trace logging for control mode/TLS decisions.
- Adds persistence of AMT Digest Realm to a per-user JSON store and expands unit tests around TLS config and leaf hash validation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/commands/diagnostics/wsman.go | Updates GetTLSConfig call to new signature. |
| internal/commands/deactivate.go | Updates GetTLSConfig call to new signature for local TLS enforced path. |
| internal/commands/deactivate_test.go | Adjusts test expectations around TLS config behavior (pre-provisioning custom verifier vs ACM default). |
| internal/commands/base.go | Updates GetTLSConfig call to new signature; adds trace logging in AfterApply. |
| internal/commands/amtinfo.go | Updates GetTLSConfig call to new signature for local WSMAN client setup. |
| internal/commands/activate/local.go | Adds UPID retrieval for ACM TLS verification; persists digest realm after reading general settings; adds trace logging. |
| internal/certs/lmsTls.go | Adds UPID-aware verification, CRL fetching/validation, improved leaf hash handling, and adjusts ROM ODCA issuer OU prefix logic. |
| internal/certs/lmsTls_test.go | Adds tests for TLS config behavior, UPID binding, and AMT leaf hash algorithm/format handling. |
| internal/certs/digestrealm_store.go | New per-user JSON store for persisting digest realm keyed by AMT UUID. |
Comments suppressed due to low confidence (1)
internal/certs/lmsTls.go:126
- VerifyCertificates fetches CRLs (HTTP requests) from CRLDistributionPoints before the certificate chain is verified. Because pre-provisioning TLS sets InsecureSkipVerify=true, rawCerts are untrusted at this point; a MITM could supply certificates with attacker-controlled CRL URLs and trigger SSRF during the handshake. Verify the chain first (against the embedded ODCA trust store) before performing any network I/O based on certificate fields (CRL/UPID binding).
if err := VerifyCRLStatus(parsedCerts); err != nil {
return err
}
if upidInfo != nil {
if romODCACert == nil {
return errors.New("failed to identify ROM ODCA certificate for UPID binding verification")
}
if err := VerifyUPIDBinding(romODCACert, upidInfo); err != nil {
return err
}
}
// verify the full chain
if err := VerifyFullChain(parsedCerts); err != nil {
return err
}
Comment on lines
+168
to
+173
| if err = crl.CheckSignatureFrom(issuer); err != nil { | ||
| lastErr = fmt.Errorf("CRL signature validation failed for %s: %w", cert.Subject.CommonName, err) | ||
|
|
||
| continue | ||
| } | ||
|
|
Comment on lines
+754
to
+765
| func (service *LocalActivationService) persistDigestRealm(digestRealm string) { | ||
| uuid, err := service.amtCommand.GetUUID() | ||
| if err != nil { | ||
| log.WithError(err).Debug("Unable to retrieve AMT UUID for digest realm persistence") | ||
|
|
||
| return | ||
| } | ||
|
|
||
| if err = certs.SaveDigestRealm(uuid, digestRealm); err != nil { | ||
| log.WithError(err).Debug("Failed to persist AMT digest realm") | ||
| } | ||
| } |
Comment on lines
+136
to
+155
| var crlHTTPClient = &http.Client{Timeout: 5 * time.Second} | ||
|
|
||
| func VerifyCRLStatus(certificates []*x509.Certificate) error { | ||
| for i, cert := range certificates { | ||
| if len(cert.CRLDistributionPoints) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| if i+1 >= len(certificates) { | ||
| continue | ||
| } | ||
|
|
||
| issuer := certificates[i+1] | ||
| if err := verifyCertAgainstCRL(cert, issuer); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
Contributor
Reason for failure being the system you tried is an engineering sample ODCA 2 CSME E_LNL Please try this on a production system or a ignore this check on a engineering sample for testing purpsoe and see how far your progress. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses - #1083
Currently the check fails as the issue name is not as expected.
error msg - "ROM ODCA Certificate OU does not have a valid prefix"
Detailed logs -