Add test enrollz-1#5484
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new test suite to validate the enrollz TPM 2.0 HMAC enrollment process. The changes include necessary infrastructure updates, such as adding the go-tpm dependency, defining a new topology binding for test environments, and extending the gnsi client interface to facilitate the new test cases. These additions ensure robust verification of the enrollment workflow and its error handling. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Pull Request Functional Test Report for #5484 / 65bbc9bVirtual Devices
Hardware Devices
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive test suite for TPM 2.0 HMAC enrollment, including a new test file, updates to the GNSI binding, and a sample topology for Arista devices. Feedback focuses on improving robustness by handling ignored errors in cryptographic and file system operations to prevent potential panics. Additionally, the review identifies several style guide violations where vendor-specific logic and CLI configurations must be abstracted into the deviations or cfgplugins packages to maintain test portability. A placeholder path in the topology binding also requires correction.
| wrongKey, _ := rsa.GenerateKey(rand.Reader, rsaKeyBits) | ||
| wrongPub, _ := x509.MarshalPKIXPublicKey(&wrongKey.PublicKey) |
There was a problem hiding this comment.
Errors from rsa.GenerateKey and x509.MarshalPKIXPublicKey are ignored. If key generation fails, wrongKey will be nil, leading to a panic. Additionally, this logic is repeated later in the file; consider refactoring into a helper function to improve maintainability.
| wrongKey, _ := rsa.GenerateKey(rand.Reader, rsaKeyBits) | |
| wrongPub, _ := x509.MarshalPKIXPublicKey(&wrongKey.PublicKey) | |
| wrongKey, err := rsa.GenerateKey(rand.Reader, rsaKeyBits) | |
| if err != nil { | |
| return fmt.Errorf("failed to generate RSA key: %w", err) | |
| } | |
| wrongPub, err := x509.MarshalPKIXPublicKey(&wrongKey.PublicKey) | |
| if err != nil { | |
| return fmt.Errorf("failed to marshal public key: %w", err) | |
| } |
References
- Simplify identical logic for different entities or repeated blocks by using a single loop or a helper function to improve maintainability.
| wrongKey, _ := rsa.GenerateKey(rand.Reader, rsaKeyBits) | ||
| wrongPub, _ := x509.MarshalPKIXPublicKey(&wrongKey.PublicKey) |
There was a problem hiding this comment.
Errors from rsa.GenerateKey and x509.MarshalPKIXPublicKey are ignored. This can lead to a nil pointer dereference if key generation fails. Since this logic is identical to the block at line 426, consider refactoring it into a helper function to improve maintainability.
| wrongKey, _ := rsa.GenerateKey(rand.Reader, rsaKeyBits) | |
| wrongPub, _ := x509.MarshalPKIXPublicKey(&wrongKey.PublicKey) | |
| wrongKey, err := rsa.GenerateKey(rand.Reader, rsaKeyBits) | |
| if err != nil { | |
| return fmt.Errorf("failed to generate RSA key: %w", err) | |
| } | |
| wrongPub, err := x509.MarshalPKIXPublicKey(&wrongKey.PublicKey) | |
| if err != nil { | |
| return fmt.Errorf("failed to marshal public key: %w", err) | |
| } |
References
- Simplify identical logic for different entities or repeated blocks by using a single loop or a helper function to improve maintainability.
| cwd, _ := os.Getwd() | ||
| enrollzCert = filepath.Join(cwd, enrollzCertFilename) | ||
| enrollzKey = filepath.Join(cwd, enrollzKeyFilename) |
There was a problem hiding this comment.
The error from os.Getwd() is ignored. It is better to handle it to avoid using an empty or incorrect path for the certificates. Using t.Fatalf is preferred here to fail fast as subsequent steps depend on this path.
| cwd, _ := os.Getwd() | |
| enrollzCert = filepath.Join(cwd, enrollzCertFilename) | |
| enrollzKey = filepath.Join(cwd, enrollzKeyFilename) | |
| cwd, err := os.Getwd() | |
| if err != nil { | |
| t.Fatalf("os.Getwd() failed: %v", err) | |
| } | |
| enrollzCert = filepath.Join(cwd, enrollzCertFilename) | |
| enrollzKey = filepath.Join(cwd, enrollzKeyFilename) |
References
- In tests, t.Fatalf is preferred over t.Errorf when a failure makes subsequent test steps meaningless, as this fails fast and reduces overall test execution time.
| } | ||
|
|
||
| func fetchPPKPubFromDut(c *ROTDBClient, ctx context.Context) (*biz.FetchEKResp, error) { | ||
| switch c.dut.Vendor() { |
There was a problem hiding this comment.
Avoid using dut.Vendor() for vendor-specific logic in tests. This logic should be abstracted using the deviations package to maintain test portability across different platforms.
References
- Avoid using "dut.Vendor()" for vendor-specific logic or configurations in tests. Instead, use the "deviations" package to maintain test abstraction and portability across different vendors.
|
|
||
| func reconfigureEnrollzService(t *testing.T, dut *ondatra.DUTDevice, sslProfileParams *sslProfileParams) { | ||
| t.Helper() | ||
| switch dut.Vendor() { |
There was a problem hiding this comment.
Vendor-specific CLI configuration should be abstracted via the deviations package or implemented as a configuration plugin (cfgplugins) to ensure the test remains vendor-neutral.
References
- Avoid using "dut.Vendor()" for vendor-specific logic or configurations in tests. Instead, use the "deviations" package to maintain test abstraction and portability across different vendors.
| } | ||
|
|
||
| func newTestROTDBClient(t *testing.T, dut *ondatra.DUTDevice, keyType epb.Key) biz.ROTDBClient { | ||
| switch dut.Vendor() { |
There was a problem hiding this comment.
Direct use of dut.Vendor() here violates the general rule of maintaining test abstraction. Please move this logic to the deviations package.
References
- Avoid using "dut.Vendor()" for vendor-specific logic or configurations in tests. Instead, use the "deviations" package to maintain test abstraction and portability across different vendors.
| gnmi { | ||
| target: "dut-hostname:6030" | ||
| mutual_tls: true | ||
| trust_bundle_file: "path-to-test-file/enrollz.crt" |
There was a problem hiding this comment.
The path path-to-test-file/enrollz.crt appears to be a placeholder. Please ensure this is replaced with a valid path or documented as a requirement for the test environment.
2 participants
Issues identified: https://partnerissuetracker.corp.google.com/issues/514403340
Logs location: https://partnerissuetracker.corp.google.com/issues/514436481