Add SCEP endpoint for host identity. - #30589
Conversation
WalkthroughThis change introduces a new SCEP (Simple Certificate Enrollment Protocol) endpoint for issuing host identity certificates at Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SCEP Endpoint (/api/fleet/orbit/host_identity/scep)
participant SCEP Service
participant Datastore
participant SCEP Depot
Client->>SCEP Endpoint: PKIOperation (CSR w/ ECC key, challenge password)
SCEP Endpoint->>SCEP Service: PKIOperation(data)
SCEP Service->>Datastore: Verify enrollment secret
Datastore-->>SCEP Service: Secret valid/invalid
alt Secret valid
SCEP Service->>SCEP Depot: Sign CSR, store cert (unique CN, ECC only)
SCEP Depot->>Datastore: Store cert, raw public key, manage revocation
SCEP Depot-->>SCEP Service: Signed certificate
SCEP Service-->>SCEP Endpoint: SCEP success response (certificate)
SCEP Endpoint-->>Client: Success (certificate)
else Secret invalid or error
SCEP Service-->>SCEP Endpoint: SCEP failure response
SCEP Endpoint-->>Client: Failure
end
Assessment against linked issues
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ee/server/service/hostidscep/scep.go (2)
91-97: Improve error message accuracyThe error message says "parsing SCEP certificate" but the operation is retrieving the certificate from storage, not parsing it.
- return nil, 0, ctxerr.Wrap(ctx, err, "parsing SCEP certificate") + return nil, 0, ctxerr.Wrap(ctx, err, "retrieving SCEP CA certificate")
108-111: Maintain error message consistencySame issue as in GetCACert - the error message should indicate retrieval, not parsing.
- return nil, ctxerr.Wrap(ctx, err, "parsing SCEP certificate") + return nil, ctxerr.Wrap(ctx, err, "retrieving SCEP CA certificate")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ee/server/service/hostidscep/config.go(1 hunks)ee/server/service/hostidscep/scep.go(1 hunks)pkg/certificate/certificate.go(2 hunks)server/datastore/mysql/scep.go(2 hunks)server/datastore/mysql/wstep.go(2 hunks)server/fleet/mdm.go(1 hunks)server/mdm/apple/cert.go(1 hunks)server/mdm/apple/util.go(0 hunks)server/mdm/scep/depot/fleet.go(1 hunks)server/service/mdm.go(3 hunks)
💤 Files with no reviewable changes (1)
- server/mdm/apple/util.go
🧰 Additional context used
🧬 Code Graph Analysis (7)
server/datastore/mysql/wstep.go (1)
pkg/certificate/certificate.go (1)
EncodeCertPEM(244-250)
server/datastore/mysql/scep.go (1)
pkg/certificate/certificate.go (1)
EncodeCertPEM(244-250)
ee/server/service/hostidscep/config.go (5)
server/contexts/license/license.go (1)
IsPremium(30-35)server/fleet/mdm.go (4)
MDMAssetName(729-729)MDMAssetHostIdentityCACert(771-771)MDMAssetHostIdentityCAKey(773-773)MDMConfigAsset(776-780)server/mdm/apple/cert.go (1)
NewSCEPCACertKey(204-206)server/mdm/scep/depot/fleet.go (1)
NewSCEPCACertKey(11-33)pkg/certificate/certificate.go (2)
EncodeCertPEM(244-250)EncodePrivateKeyPEM(253-259)
server/mdm/scep/depot/fleet.go (2)
server/mdm/apple/cert.go (1)
NewSCEPCACertKey(204-206)pkg/certificate/certificate.go (1)
Certificate(132-136)
ee/server/service/hostidscep/scep.go (3)
server/fleet/datastore.go (2)
MDMAppleStore(2226-2231)MDMAssetRetriever(2233-2237)server/mdm/scep/server/endpoint.go (2)
MakeServerEndpoints(98-104)EndpointLoggingMiddleware(215-231)server/mdm/scep/server/transport.go (1)
MakeHTTPHandler(20-41)
server/service/mdm.go (2)
pkg/certificate/certificate.go (2)
EncodeCertPEM(244-250)EncodePrivateKeyPEM(253-259)server/fleet/mdm.go (3)
MDMAssetCACert(734-734)MDMAssetCAKey(737-737)MDMAssetAPNSKey(740-740)
server/mdm/apple/cert.go (1)
server/mdm/scep/depot/fleet.go (1)
NewSCEPCACertKey(11-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (main, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (service, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (service, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: build-binaries
- GitHub Check: test-packaging (ubuntu-latest)
- GitHub Check: test-go (fast, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-packaging (macos-15)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (main, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: Analyze (go)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: Analyze (javascript)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: publish
- GitHub Check: test-packaging (ubuntu-latest, local)
- GitHub Check: test-packaging (ubuntu-latest, remote)
- GitHub Check: test-preview (ubuntu-latest)
- GitHub Check: check-doc-gen
- GitHub Check: lint (windows-latest)
- GitHub Check: lint (macos-latest)
- GitHub Check: lint (ubuntu-latest)
- GitHub Check: test-db-changes
🔇 Additional comments (13)
server/datastore/mysql/wstep.go (1)
12-12: LGTM: Clean refactoring to centralized certificate packageThe import change and function call update correctly use the new centralized
certificate.EncodeCertPEMfunction, which provides identical functionality to the previous Apple MDM-specific implementation.Also applies to: 31-31
server/datastore/mysql/scep.go (1)
14-14: LGTM: Consistent refactoring patternThe changes correctly update the import and function call to use the centralized
certificate.EncodeCertPEMfunction, maintaining the same functionality while following the consistent refactoring pattern across the codebase.Also applies to: 93-93
server/fleet/mdm.go (1)
770-773: LGTM: Well-named constants for host identity SCEPThe new constants follow the established naming pattern and provide clear, descriptive names for the host identity CA certificate and key assets. This supports the new host identity SCEP functionality mentioned in the PR objectives.
server/mdm/apple/cert.go (1)
205-205: LGTM: Proper delegation to centralized implementationThe refactoring correctly delegates to
depot.NewSCEPCACertKey(), which centralizes the SCEP CA certificate/key generation logic while maintaining the same function signature and behavior.pkg/certificate/certificate.go (2)
6-6: LGTM: Required imports for PEM encoding functionsThe new imports for
crypto/rsaandencoding/pemare correctly added to support the new PEM encoding functions.Also applies to: 9-9
243-259: LGTM: Well-implemented centralized PEM encoding functionsThe new
EncodeCertPEMandEncodePrivateKeyPEMfunctions correctly implement PEM encoding:
- Uses standard PEM block types ("CERTIFICATE" and "RSA PRIVATE KEY")
- Proper use of
x509.MarshalPKCS1PrivateKeyfor RSA keys- Consistent with the previous Apple MDM-specific implementations
- Centralizes this functionality for reuse across the codebase
This successfully consolidates PEM encoding logic that was previously scattered across different packages.
server/service/mdm.go (3)
24-24: LGTM: Import addition supports refactoringThe new certificate package import is appropriately added to support the refactored PEM encoding functions.
213-215: LGTM: Clean refactoring to generic encoding functionsThe replacement of Apple MDM-specific PEM encoding functions with generic certificate package functions improves code modularity while maintaining identical functionality.
2529-2531: LGTM: Consistent refactoring of encoding functionsThe PEM encoding function calls are consistently updated to use the generic certificate package, completing the refactoring started in the RequestMDMAppleCSR method.
server/mdm/scep/depot/fleet.go (2)
35-41: LGTM: Secure key generation with appropriate key sizeThe RSA key size of 2048 bits correctly addresses Apple's CSR requirements. The use of
crypto/randensures cryptographically secure random number generation for the private key.
11-33: Certificate creation helper functions verified and approvedThe
NewCACert,WithYears, andWithCommonNamefunctions are properly implemented inserver/mdm/scep/depot/cacert.go, so the CA certificate generation logic inNewSCEPCACertKeyis sound. No further changes required.ee/server/service/hostidscep/config.go (1)
30-52: No additional uniqueness safeguards needed—DB constraint already prevents duplicatesThe
mdm_config_assetstable defines a unique index on (name, deletion_uuid), andInsertMDMConfigAssetswill error if you try to insert the same asset name twice. Becausedeletion_uuiddefaults to the empty string, any second insert of the same name will hit that unique‐key violation.Rather than adding a database constraint, you should catch and ignore “duplicate key” errors around the insert to make the init routine safe for concurrent startup. For example:
if err := ds.InsertMDMConfigAssets(ctx, assets, nil); err != nil { // If the error is a MySQL/SQLite duplicate‐key violation, ignore it if !isDuplicateKeyErr(err) { return fmt.Errorf("inserting host identity SCEP assets: %w", err) } }• Add an
isDuplicateKeyErr(error) boolhelper that inspects the SQL error code for a unique‐constraint violation.
• Leave the existing UNIQUE INDEX (idx_mdm_config_assets_name_deletion_uuid) in place.This change ensures only one instance creates the assets while others simply proceed when they observe “already exists.”
ee/server/service/hostidscep/scep.go (1)
66-89: Well-designed capability declarationThe detailed documentation of supported and unsupported capabilities is excellent. The chosen capabilities (SHA-256, AES, POSTPKIOperation) represent secure, modern choices for SCEP implementation.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #30589 +/- ##
==========================================
+ Coverage 64.11% 64.15% +0.03%
==========================================
Files 1878 1886 +8
Lines 184499 184873 +374
Branches 5400 5400
==========================================
+ Hits 118297 118603 +306
- Misses 56897 56942 +45
- Partials 9305 9328 +23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
server/fleet/host_identity_certificates.go (1)
20-35: Redundant validation logic in UnmarshalPublicKey method.The method performs two separate checks that could be consolidated:
- Line 21:
guessCurvefunction checks raw byte length and returns error for unsupported lengths- Line 26: Direct check for empty array and 0x04 prefix
The
guessCurvefunction will already return an error for empty arrays, making thelen(h.PublicKeyRaw) == 0check redundant.Consider consolidating the validation logic:
func (h *HostIdentityCertificate) UnmarshalPublicKey() (*ecdsa.PublicKey, error) { + if len(h.PublicKeyRaw) == 0 || h.PublicKeyRaw[0] != 4 { + return nil, errors.New("unsupported EC point format") + } + curve, err := guessCurve(h.PublicKeyRaw) if err != nil { return nil, err } - if len(h.PublicKeyRaw) == 0 || h.PublicKeyRaw[0] != 4 { // 0x04 means this is the raw representation - return nil, errors.New("unsupported EC point format") - } - byteLen := (len(h.PublicKeyRaw) - 1) / 2 x := new(big.Int).SetBytes(h.PublicKeyRaw[1 : 1+byteLen]) y := new(big.Int).SetBytes(h.PublicKeyRaw[1+byteLen:])ee/server/integrationtest/hostidscep/hostidscep_test.go (2)
42-50: Consider adding P521 curve test for comprehensive coverage.The test covers P256 and P384 curves but misses P521, which is also a standard NIST curve commonly used in certificate generation.
func testGetCert(t *testing.T, s *Suite) { t.Run("ECC P256", func(t *testing.T) { testGetCertWithCurve(t, s, elliptic.P256()) }) t.Run("ECC P384", func(t *testing.T) { testGetCertWithCurve(t, s, elliptic.P384()) }) + + t.Run("ECC P521", func(t *testing.T) { + testGetCertWithCurve(t, s, elliptic.P521()) + }) }
87-88: Consider using a stronger RSA key size for enhanced security.While 2048-bit RSA is currently the minimum recommended size, using 3072 or 4096 bits would provide better security margins for the future.
// Create temporary RSA key for SCEP envelope (required by SCEP protocol) - tempRSAKey, err := rsa.GenerateKey(rand.Reader, 2048) + tempRSAKey, err := rsa.GenerateKey(rand.Reader, 3072) require.NoError(t, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
cmd/fleet/serve.go(2 hunks)ee/server/integrationtest/hostidscep/hostidscep_test.go(1 hunks)ee/server/integrationtest/hostidscep/suite.go(1 hunks)ee/server/service/hostidscep/config.go(1 hunks)ee/server/service/hostidscep/scep.go(1 hunks)server/datastore/mysql/common_mysql/testing_utils/testing_utils.go(2 hunks)server/datastore/mysql/host_identity_scep.go(1 hunks)server/datastore/mysql/migrations/tables/20250707095725_HostIdentitySCEPCertificates.go(1 hunks)server/datastore/mysql/mysql.go(1 hunks)server/datastore/mysql/schema.sql(2 hunks)server/fleet/datastore.go(1 hunks)server/fleet/host_identity_certificates.go(1 hunks)server/fleet/host_identity_certificates_test.go(1 hunks)server/mock/datastore_mock.go(3 hunks)server/service/integrationtest/suite.go(2 hunks)server/service/testing_utils.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/server/service/hostidscep/scep.go
🧰 Additional context used
🧬 Code Graph Analysis (6)
server/fleet/datastore.go (1)
server/fleet/host_identity_certificates.go (1)
HostIdentityCertificate(12-18)
cmd/fleet/serve.go (1)
ee/server/service/hostidscep/scep.go (1)
RegisterSCEP(27-57)
server/service/testing_utils.go (1)
ee/server/service/hostidscep/scep.go (1)
RegisterSCEP(27-57)
server/service/integrationtest/suite.go (2)
server/datastore/mysql/testing_utils.go (1)
CreateMySQLDSWithOptions(394-396)server/datastore/mysql/common_mysql/testing_utils/testing_utils.go (1)
DatastoreTestOptions(82-99)
server/fleet/host_identity_certificates_test.go (1)
server/fleet/host_identity_certificates.go (2)
CreateECDSAPublicKeyRaw(48-67)HostIdentityCertificate(12-18)
server/mock/datastore_mock.go (2)
server/fleet/host_identity_certificates.go (1)
HostIdentityCertificate(12-18)server/fleet/calendar.go (1)
Lock(59-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (33)
- GitHub Check: publish
- GitHub Check: test-go (service, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (scripts, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (main, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (service, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (main, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (fast, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: check-doc-gen
- GitHub Check: test-packaging (macos-15)
- GitHub Check: test-db-changes
- GitHub Check: test-packaging (ubuntu-latest)
- GitHub Check: lint (windows-latest)
- GitHub Check: build-and-check
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript)
- GitHub Check: test-packaging (ubuntu-latest, remote)
- GitHub Check: lint (ubuntu-latest)
- GitHub Check: lint (macos-latest)
- GitHub Check: test-preview (ubuntu-latest)
- GitHub Check: test-packaging (ubuntu-latest, local)
- GitHub Check: build-binaries
🔇 Additional comments (25)
server/fleet/datastore.go (1)
2206-2211: LGTM! Well-designed interface addition for Host Identity SCEP support.This method addition follows established datastore patterns with clear documentation and appropriate type signatures. The integration with the broader Host Identity SCEP feature is well-structured and maintains consistency with existing certificate management interfaces.
server/service/testing_utils.go (3)
21-21: LGTM: Import correctly added for Host Identity SCEP functionality.The import is properly placed and necessary for the new SCEP service registration.
367-367: LGTM: New storage field follows established patterns.The
HostIdentitySCEPStoragefield is consistent with other storage options in the struct and uses the appropriatescep_depot.Depottype.
475-477: LGTM: Conditional SCEP service registration follows established patterns.The implementation correctly:
- Uses conditional registration only when storage is provided
- Passes the expected parameters (mux, storage, datastore, logger) to
hostidscep.RegisterSCEP- Follows the same pattern as other conditional service registrations in this function
server/datastore/mysql/mysql.go (2)
192-196: LGTM: New Host Identity SCEP depot method is well-implemented.The
NewHostIdentitySCEPDepotmethod:
- Follows the same pattern as the existing
NewSCEPDepotmethod- Appropriately takes an additional logger parameter for specialized logging needs
- Returns the correct interface type
- Properly delegates to the underlying implementation function
186-190: No remaining references toNewMDMAppleSCEPDepot.
A repository-wide search confirmed zero occurrences of the old method name and all callers now useNewSCEPDepot. The rename is safe to merge.cmd/fleet/serve.go (2)
28-28: Import addition looks good.The import is correctly added and aligns with the new Host Identity SCEP feature implementation.
1197-1209: Well-structured conditional feature activation.The Host Identity SCEP setup follows the established pattern for premium features:
- Properly gated behind premium license check
- Conditional activation based on private key configuration
- Appropriate error handling with
initFatal- Helpful warning message when prerequisites aren't met
- Consistent with existing SCIM and SCEP proxy registration patterns
The security consideration of requiring a private key before enabling certificate-related functionality is appropriate.
ee/server/service/hostidscep/config.go (1)
12-50: Robust and secure asset initialization implementation.The
initAssetsfunction demonstrates several good practices:
- Idempotent design: Checks for existing assets before generating new ones, preventing unnecessary regeneration
- Proper error handling: Uses error wrapping with context for debugging
- Security: Stores assets as encrypted MDM config assets in the database
- Established patterns: Uses the same asset management approach as other MDM components
The logic correctly handles the "not found" case for first-time asset generation while properly surfacing other database errors.
server/service/integrationtest/suite.go (2)
10-10: Import addition supports enhanced test infrastructure.The testing_utils import enables the improved datastore creation with explicit test naming options.
57-59: Improved test isolation through explicit naming.The change from
CreateMySQLDStoCreateMySQLDSWithOptionswithUniqueTestNameprovides better test isolation by allowing explicit control over test database naming. This prevents potential conflicts between concurrent tests and supports the new Host Identity SCEP integration test requirements.server/datastore/mysql/common_mysql/testing_utils/testing_utils.go (2)
98-98: Enhanced test configuration with explicit naming option.The addition of
UniqueTestNamefield provides flexibility for test setups that require explicit control over database naming while maintaining backwards compatibility.
187-201: Well-implemented fallback logic for test naming.The logic correctly prioritizes explicit
UniqueTestNamewhen provided, falling back to the existing runtime-derived naming approach. This maintains full backwards compatibility while enabling better test isolation for scenarios like the new Host Identity SCEP integration tests.ee/server/integrationtest/hostidscep/suite.go (1)
15-51: Comprehensive integration test suite setup.The
Suitestruct andSetUpSuitefunction provide a robust foundation for Host Identity SCEP integration testing:
- Proper licensing: Correctly sets up premium license tier for the feature
- Complete infrastructure: Creates MySQL datastore, SCEP depot, and test server with all dependencies
- Established patterns: Follows the same structure as other integration test suites in the codebase
- Test isolation: Uses unique test naming to prevent conflicts
- Authentication setup: Configures admin token for test execution
The setup ensures the full Host Identity SCEP flow can be tested end-to-end with proper isolation and dependencies.
server/mock/datastore_mock.go (3)
1415-1415: LGTM: Function type declaration follows established patterns.The function type declaration is correctly implemented with appropriate parameter types that match the
SerialNumberfield type from theHostIdentityCertificatestruct.
3503-3504: LGTM: Struct fields follow established naming conventions.The new fields are properly named and positioned, following the consistent pattern used throughout the mock datastore for tracking mock function invocations.
8374-8379: LGTM: Method implementation follows standard mock pattern.The method correctly implements the standard mock pattern with proper mutex handling for thread safety and invocation tracking. The implementation is consistent with other mock methods in the file.
server/fleet/host_identity_certificates_test.go (1)
1-122: Excellent comprehensive test coverage for ECDSA public key handling.The test suite provides thorough coverage of both successful operations and error conditions for the
HostIdentityCertificatepublic key functionality. The tests are well-structured with clear naming conventions and appropriate use of helper functions to avoid duplication.Key strengths:
- Tests both supported curves (P256, P384) and unsupported curve (P521)
- Validates proper error handling for various malformed inputs
- Uses realistic key generation for authentic test scenarios
- Helper function
testUnmarshalPublicKeyWithCurvepromotes code reuseserver/fleet/host_identity_certificates.go (3)
12-18: Well-designed struct for host identity certificate storage.The
HostIdentityCertificatestruct is appropriately designed with database tags and includes all necessary fields for certificate management including serial number, common name, host association, validity period, and raw public key storage.
37-46: Efficient curve detection based on byte length.The
guessCurvefunction provides a clean mapping from raw byte lengths to elliptic curves, supporting the standard uncompressed point format lengths for P256 and P384.
48-67: Robust ECDSA public key encoding with proper padding.The
CreateECDSAPublicKeyRawfunction correctly handles the conversion from ECDSA public keys to raw byte format with appropriate coordinate padding usingFillBytes()to ensure consistent byte lengths.server/datastore/mysql/migrations/tables/20250707095725_HostIdentitySCEPCertificates.go (3)
12-24: Well-designed serial number table with appropriate CA reservation.The
host_identity_scep_serialstable design is solid with auto-increment starting at 2 to reserve serial number 1 for the CA certificate, which follows SCEP best practices.
26-48: Comprehensive certificate storage table with proper constraints.The
host_identity_scep_certificatestable design includes all necessary fields for certificate management:
- Proper foreign key relationship to serials table
- Efficient indexes on
nameandhost_idfor lookup and revocation operations- Appropriate data types for certificate storage
- Check constraint to validate PEM format integrity
The
public_key_raw VARBINARY(100)field size accommodates both P256 (65 bytes) and P384 (97 bytes) public keys with room for future expansion.
53-66: Proper rollback implementation respecting foreign key constraints.The
Downfunction correctly drops tables in reverse order to respect foreign key constraints, ensuring clean migration rollback.server/datastore/mysql/schema.sql (1)
521-525: Serial table LGTM, tiny nitTable definition is solid. If you rely on reproducible schema dumps, explicitly setting
AUTO_INCREMENT=1(as done in a few other tables) keeps tools quiet.
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
server/datastore/mysql/migrations/tables/20250707095725_HostIdentitySCEPCertificates.go (1)
53-55: Empty Down function - consider adding implementationThe
Downfunction is currently empty, which means the migration cannot be rolled back. While this might be intentional for safety reasons, consider whether rollback capability would be useful for development or emergency scenarios.If rollback capability is desired, consider implementing:
func Down_20250707095725(tx *sql.Tx) error { - return nil + // Drop tables in reverse order to handle foreign key constraints + if _, err := tx.Exec("DROP TABLE IF EXISTS host_identity_scep_certificates"); err != nil { + return fmt.Errorf("failed to drop host_identity_scep_certificates table: %w", err) + } + if _, err := tx.Exec("DROP TABLE IF EXISTS host_identity_scep_serials"); err != nil { + return fmt.Errorf("failed to drop host_identity_scep_serials table: %w", err) + } + return nil }ee/server/service/hostidentity/depot/depot.go (2)
58-69: Add comment to clarify auto-increment behavior.The empty VALUES clause relies on MySQL's auto-increment behavior. Consider adding a comment to make this explicit.
// Serial allocates and returns a new (increasing) serial number. func (d *HostIdentitySCEPDepot) Serial() (*big.Int, error) { + // Insert an empty row to generate a new auto-incremented serial number result, err := d.db.Exec(`INSERT INTO host_identity_scep_serials () VALUES ();`)
71-75: Document the stub implementation more clearly.This method always returns false regardless of parameters. If callers depend on this behavior, it could lead to unexpected results. Consider adding a more explicit comment or returning an "not implemented" error.
// HasCN returns whether the given certificate exists in the depot. func (d *HostIdentitySCEPDepot) HasCN(cn string, allowTime int, cert *x509.Certificate, revokeOldCertificate bool) (bool, error) { - // Not used right now. May be used for renewal in the future. + // TODO: Not implemented. Always returns false. + // This will be implemented when certificate renewal is supported. return false, nil }ee/server/service/hostidentity/scep.go (3)
55-59: Track missing monitoring and error reporting.The comment indicates that APM/OpenTel monitoring and error reporting to Sentry/Redis are missing. This should be tracked properly.
Would you like me to create an issue to track the implementation of proper monitoring and error reporting for the SCEP server?
132-135: Consider more specific validation for PKIOperation data.While checking for empty data is good, consider validating minimum size requirements for a valid SCEP message.
func (svc *service) PKIOperation(ctx context.Context, data []byte) ([]byte, error) { - if len(data) == 0 { - return nil, &fleet.BadRequestError{Message: "missing data for PKIOperation"} + // Minimum size for a valid SCEP message is typically larger than a few bytes + const minSCEPMessageSize = 100 // Adjust based on actual SCEP message requirements + if len(data) < minSCEPMessageSize { + return nil, &fleet.BadRequestError{Message: "invalid or missing data for PKIOperation"} }
155-158: Provide more specific error message.When the signer returns nil certificate without error, the generic error message could be more descriptive.
crt, err := svc.signer.SignCSRContext(ctx, msg.CSRReqMessage) if err == nil && crt == nil { - err = errors.New("no signed certificate") + err = errors.New("signer returned nil certificate without error") }ee/server/integrationtest/hostidscep/hostidscep_test.go (3)
99-100: Extract magic numbers to test constants.RSA key size and other values are hardcoded throughout the tests.
Add these constants at the package level:
const ( testRSAKeySize = 2048 testCertValidityDays = 365 )Then update the usages:
- tempRSAKey, err := rsa.GenerateKey(rand.Reader, 2048) + tempRSAKey, err := rsa.GenerateKey(rand.Reader, testRSAKeySize)Also applies to: 272-273, 295-296
162-166: Add more specific assertion messages.The assertions could benefit from more descriptive error messages to aid debugging.
- assert.Equal(t, x509.ECDSA, cert.PublicKeyAlgorithm) + assert.Equal(t, x509.ECDSA, cert.PublicKeyAlgorithm, "Certificate should use ECDSA public key algorithm") certPubKey, ok := cert.PublicKey.(*ecdsa.PublicKey) require.True(t, ok, "Certificate should contain ECC public key") - assert.True(t, eccPrivateKey.PublicKey.Equal(certPubKey), "Certificate public key should match our ECC private key") + assert.True(t, eccPrivateKey.PublicKey.Equal(certPubKey), "Certificate public key should match the CSR's ECC public key")
342-343: Remove redundant assertion.The second assertion is redundant as it's just the negation of the first.
// Verify failure response assert.Equal(t, scep.FAILURE, pkiMsgResp.PKIStatus, "SCEP request should fail") - assert.NotEqual(t, scep.SUCCESS, pkiMsgResp.PKIStatus, "SCEP request should not succeed")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Makefile(1 hunks)cmd/fleet/serve.go(2 hunks)ee/server/integrationtest/hostidscep/hostidscep_test.go(1 hunks)ee/server/service/hostidentity/config.go(1 hunks)ee/server/service/hostidentity/depot/depot.go(1 hunks)ee/server/service/hostidentity/scep.go(1 hunks)ee/server/service/hostidentity/types/host_identity_certificates.go(1 hunks)ee/server/service/hostidentity/types/host_identity_certificates_test.go(1 hunks)server/cron/calendar_cron_test.go(0 hunks)server/datastore/mysql/host_identity_scep.go(1 hunks)server/datastore/mysql/migrations/tables/20250707095725_HostIdentitySCEPCertificates.go(1 hunks)server/datastore/mysql/mysql.go(2 hunks)server/fleet/datastore.go(2 hunks)server/mock/datastore_mock.go(4 hunks)server/service/testing_utils.go(3 hunks)
💤 Files with no reviewable changes (1)
- server/cron/calendar_cron_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- server/service/testing_utils.go
- server/datastore/mysql/host_identity_scep.go
- cmd/fleet/serve.go
- server/datastore/mysql/mysql.go
- server/fleet/datastore.go
- server/mock/datastore_mock.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.721Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
ee/server/service/hostidentity/types/host_identity_certificates_test.go (1)
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.721Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
server/datastore/mysql/migrations/tables/20250707095725_HostIdentitySCEPCertificates.go (1)
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.721Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
ee/server/service/hostidentity/types/host_identity_certificates.go (1)
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.721Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
ee/server/integrationtest/hostidscep/hostidscep_test.go (1)
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.721Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
🧬 Code Graph Analysis (1)
ee/server/service/hostidentity/types/host_identity_certificates.go (1)
server/datastore/mysql/mysql.go (1)
New(222-275)
🪛 GitHub Actions: CodeQL
Makefile
[error] 191-191: Build failed during 'make fleet' command: build constraints exclude all Go files in 'server/bindata' directory.
🔇 Additional comments (10)
Makefile (1)
298-298: LGTM - Proper integration of new package into test suiteThe addition of the new
hostidentity/typespackage to the fast test suite is appropriate and ensures the unit tests for the new Host Identity SCEP functionality are run efficiently in CI.ee/server/service/hostidentity/config.go (1)
12-50: LGTM - Well-structured asset initialization functionThe
initAssetsfunction is well-implemented with proper error handling and clear logic flow. The function appropriately:
- Checks for existing assets before generating new ones
- Handles not-found errors gracefully for first-time initialization
- Generates new SCEP CA certificate and key when needed
- Uses the new generic certificate encoding functions
- Stores assets securely with proper error wrapping
The use of
context.Background()is acceptable for initialization tasks, and the function being unexported is appropriate for internal use.ee/server/service/hostidentity/types/host_identity_certificates_test.go (2)
13-97: Excellent test coverage for public key handlingThe test suite comprehensively covers the
UnmarshalPublicKeyfunctionality with:
- Positive cases: Both supported curves (P256, P384) are tested
- Error conditions: Unsupported curves, invalid formats, empty data, wrong lengths
- Edge cases: Missing 0x04 prefix, wrong prefix values
- Proper assertions: Verifies both error conditions and successful key reconstruction
The use of a helper function
testUnmarshalPublicKeyWithCurvefor testing supported curves reduces code duplication and ensures consistent testing across different curve types.
99-122: Good test helper design with thorough verificationThe
testUnmarshalPublicKeyWithCurvehelper function properly:
- Generates test keys with the specified curve
- Converts to raw format and back
- Verifies all key properties match (curve, X/Y coordinates)
- Uses both
CmpandEqualmethods for comprehensive validationThe verification approach is thorough and ensures the round-trip conversion maintains key integrity.
ee/server/service/hostidentity/types/host_identity_certificates.go (4)
12-18: Well-designed struct with appropriate database tagsThe
HostIdentityCertificatestruct is well-designed with:
- Clear field mapping: Database tags correctly map to expected column names
- Appropriate types: Uses
uint64for serial numbers and*uintfor nullable host_id- Raw key storage:
PublicKeyRawfield stores the uncompressed EC point formatThe struct design aligns with the database schema and provides the necessary fields for certificate management.
20-35: Robust public key unmarshaling with proper validationThe
UnmarshalPublicKeymethod is well-implemented:
- Format validation: Correctly checks for 0x04 prefix indicating uncompressed format
- Curve detection: Uses key length to determine the appropriate elliptic curve
- Coordinate extraction: Properly splits the raw bytes into X and Y coordinates
- Error handling: Returns descriptive errors for invalid formats
The implementation follows standard EC public key parsing practices and handles edge cases appropriately.
37-46: Simple and effective curve detection logicThe
guessCurvefunction correctly identifies curves based on raw key length:
- P256: 65 bytes (0x04 + 32 + 32)
- P384: 97 bytes (0x04 + 48 + 48)
The logic is straightforward and provides clear error messages for unsupported key lengths.
48-67: Proper raw key creation with coordinate paddingThe
CreateECDSAPublicKeyRawfunction correctly:
- Validates curves: Only supports P256 and P384 as intended
- Pads coordinates: Uses
FillBytesto ensure proper padding to expected size- Adds prefix: Correctly prefixes with 0x04 for uncompressed format
- Error handling: Returns descriptive errors for unsupported curves
The implementation ensures consistent raw key format across the system.
server/datastore/mysql/migrations/tables/20250707095725_HostIdentitySCEPCertificates.go (1)
12-51: Well-designed database schema with proper constraintsThe migration creates a robust schema for Host Identity SCEP certificates:
Strengths:
- Proper table ordering: Creates
host_identity_scep_serialsfirst to satisfy foreign key constraints- Smart serial management: AUTO_INCREMENT=2 reserves serial 1 for CA certificate
- Appropriate indexing: Indexes on
nameandhost_idfor efficient lookups and revocation- Data integrity: Foreign key constraints and PEM format validation via check constraint
- Good data types: Uses appropriate MySQL types with proper charset/collation
Schema design choices (per retrieved learnings, these are intentional):
VARBINARY(100)for public_key_raw storage- Nullable
host_idwithout foreign key constraintDATETIMEinstead ofDATETIME(6)for validity timestampsee/server/service/hostidentity/depot/depot.go (1)
31-41: LGTM!The constructor properly validates the database connection before returning the depot instance.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
ee/server/service/hostidentity/depot/depot.go (4)
104-119: Security concern: Shared challenge allows certificate revocation.
108-111: Use the provided context instead of context.Background().Also applies to: 120-124
82-82: Extract magic number to a constant.
115-115: Handle the error from RowsAffected().
🧹 Nitpick comments (6)
server/service/mdm.go (1)
213-215:EncodePrivateKeyPEMis RSA-only – consider future-proofing
certificate.EncodePrivateKeyPEMaccepts*rsa.PrivateKey; if we ever migrate SCEP/MDM keys to ECC (P-256/P-384) this call will not compile.Optional improvement:
- func EncodePrivateKeyPEM(key *rsa.PrivateKey) []byte { + func EncodePrivateKeyPEM(key crypto.PrivateKey) []byte { + switch k := key.(type) { + case *rsa.PrivateKey: + return pem.EncodeToMemory(&pem.Block{ + Type: "RSA PRIVATE KEY", + Bytes: x509.MarshalPKCS1PrivateKey(k), + }) + case *ecdsa.PrivateKey: + b, _ := x509.MarshalECPrivateKey(k) + return pem.EncodeToMemory(&pem.Block{ + Type: "EC PRIVATE KEY", + Bytes: b, + }) + default: + return nil + } }This keeps callers unchanged and removes the RSA assumption.
server/service/integrationtest/suite.go (1)
57-59: Nice isolation of test databasesCreating the datastore with
UniqueTestNamehelps parallel test runs avoid collisions. One nit: if callers pass an emptyuniqueTestName, the DB name becomes an empty suffix. Consider auto-generating a UUID when the string is blank to keep the invariant that each suite gets its own DB.ee/server/integrationtest/hostidscep/hostidscep_test.go (3)
24-24: Consider using a more descriptive constant name.While the constant is fine for tests, consider using a more descriptive name like
testEnrollmentSecretto make it clearer that this is test data.-const enrollmentSecret = "test_secret" +const testEnrollmentSecret = "test_secret"
98-125: Verify the necessity of temporary RSA key creation for each test.While the temporary RSA key is required by the SCEP protocol for envelope encryption, consider extracting this logic into a helper function to reduce code duplication between success and failure tests.
// Helper function to create temporary RSA key and certificate for SCEP envelope func createTempRSAKeyAndCert(t *testing.T, commonName string) (*rsa.PrivateKey, *x509.Certificate) { tempRSAKey, err := rsa.GenerateKey(rand.Reader, 2048) require.NoError(t, err) deviceCertTemplate := x509.Certificate{ Subject: pkix.Name{ CommonName: commonName, }, NotBefore: time.Now(), NotAfter: time.Now().Add(365 * 24 * time.Hour), KeyUsage: KeyUsageKeyEncryption | KeyUsageDigitalSignature, ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, BasicConstraintsValid: true, } deviceCertDerBytes, err := x509.CreateCertificate( rand.Reader, &deviceCertTemplate, &deviceCertTemplate, &tempRSAKey.PublicKey, tempRSAKey, ) require.NoError(t, err) deviceCert, err := x509.ParseCertificate(deviceCertDerBytes) require.NoError(t, err) return tempRSAKey, deviceCert }
272-276: Consider adding a comment about RSA key usage in failure tests.While the test correctly uses RSA keys to test non-ECC algorithm rejection, adding a comment would clarify that this is intentional for testing the server's algorithm validation.
} else { - // Create RSA private key (should fail) + // Create RSA private key to test non-ECC algorithm rejection (should fail) rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) require.NoError(t, err) privateKey = rsaKey sigAlg = x509.SHA256WithRSA }server/datastore/mysql/schema.sql (1)
501-517: Nit: misleading index prefixidx_host_id_scep_name.The prefix
idx_host_id_…implies thathost_idis part of the indexed columns, yet the index only coversname. This can be confusing when debugging execution plans later on.- KEY `idx_host_id_scep_name` (`name`), + -- keep naming consistent with indexed columns + KEY `idx_scep_name` (`name`),Purely cosmetic, no functional impact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
Makefile(1 hunks)cmd/fleet/serve.go(2 hunks)ee/server/integrationtest/hostidscep/hostidscep_test.go(1 hunks)ee/server/integrationtest/hostidscep/suite.go(1 hunks)ee/server/service/hostidentity/config.go(1 hunks)ee/server/service/hostidentity/depot/depot.go(1 hunks)ee/server/service/hostidentity/scep.go(1 hunks)ee/server/service/hostidentity/types/host_identity_certificates.go(1 hunks)ee/server/service/hostidentity/types/host_identity_certificates_test.go(1 hunks)pkg/certificate/certificate.go(2 hunks)server/cron/calendar_cron_test.go(0 hunks)server/datastore/mysql/common_mysql/testing_utils/testing_utils.go(2 hunks)server/datastore/mysql/host_identity_scep.go(1 hunks)server/datastore/mysql/migrations/tables/20250707095725_HostIdentitySCEPCertificates.go(1 hunks)server/datastore/mysql/mysql.go(2 hunks)server/datastore/mysql/scep.go(2 hunks)server/datastore/mysql/schema.sql(2 hunks)server/datastore/mysql/wstep.go(2 hunks)server/fleet/datastore.go(2 hunks)server/fleet/mdm.go(1 hunks)server/mdm/apple/cert.go(1 hunks)server/mdm/apple/util.go(0 hunks)server/mdm/scep/depot/fleet.go(1 hunks)server/mock/datastore_mock.go(4 hunks)server/service/integrationtest/suite.go(2 hunks)server/service/mdm.go(3 hunks)server/service/testing_utils.go(3 hunks)
💤 Files with no reviewable changes (2)
- server/cron/calendar_cron_test.go
- server/mdm/apple/util.go
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.721Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
ee/server/service/hostidentity/types/host_identity_certificates_test.go (1)
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.721Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
server/datastore/mysql/migrations/tables/20250707095725_HostIdentitySCEPCertificates.go (1)
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.721Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
server/datastore/mysql/host_identity_scep.go (1)
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.721Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
ee/server/integrationtest/hostidscep/hostidscep_test.go (1)
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.721Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
ee/server/service/hostidentity/types/host_identity_certificates.go (1)
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.721Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
server/datastore/mysql/schema.sql (1)
undefined
<retrieved_learning>
Learnt from: getvictor
PR: #30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.721Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
</retrieved_learning>
🧬 Code Graph Analysis (11)
server/datastore/mysql/scep.go (1)
pkg/certificate/certificate.go (1)
EncodeCertPEM(244-250)
server/fleet/datastore.go (1)
ee/server/service/hostidentity/types/host_identity_certificates.go (1)
HostIdentityCertificate(12-18)
server/mock/datastore_mock.go (2)
ee/server/service/hostidentity/types/host_identity_certificates.go (1)
HostIdentityCertificate(12-18)server/fleet/calendar.go (1)
Lock(59-77)
server/mdm/apple/cert.go (1)
server/mdm/scep/depot/fleet.go (1)
NewSCEPCACertKey(11-33)
server/datastore/mysql/wstep.go (1)
pkg/certificate/certificate.go (1)
EncodeCertPEM(244-250)
cmd/fleet/serve.go (2)
ee/server/service/hostidentity/depot/depot.go (1)
NewHostIdentitySCEPDepot(32-41)ee/server/service/hostidentity/scep.go (1)
RegisterSCEP(27-63)
server/service/testing_utils.go (1)
ee/server/service/hostidentity/scep.go (1)
RegisterSCEP(27-63)
server/service/integrationtest/suite.go (2)
server/datastore/mysql/testing_utils.go (1)
CreateMySQLDSWithOptions(394-396)server/datastore/mysql/common_mysql/testing_utils/testing_utils.go (1)
DatastoreTestOptions(82-99)
server/service/mdm.go (2)
pkg/certificate/certificate.go (2)
EncodeCertPEM(244-250)EncodePrivateKeyPEM(253-259)server/fleet/mdm.go (3)
MDMAssetCACert(734-734)MDMAssetCAKey(737-737)MDMAssetAPNSKey(740-740)
server/mdm/scep/depot/fleet.go (2)
server/mdm/apple/cert.go (1)
NewSCEPCACertKey(204-206)pkg/certificate/certificate.go (1)
Certificate(132-136)
ee/server/service/hostidentity/types/host_identity_certificates.go (1)
server/datastore/mysql/mysql.go (1)
New(222-275)
🪛 GitHub Actions: CodeQL
Makefile
[error] 191-191: Make build failed: build constraints exclude all Go files in server/bindata directory.
🔇 Additional comments (43)
server/datastore/mysql/wstep.go (2)
12-12: LGTM: Import change aligns with certificate encoding centralization.The replacement of the Apple MDM-specific import with the generic certificate package is part of a well-structured refactoring effort.
31-31: LGTM: Function call correctly updated to use centralized encoding.The change from
apple_mdm.EncodeCertPEM(crt)tocertificate.EncodeCertPEM(crt)is consistent with the import change and maintains the same functionality while using the centralized certificate encoding utilities.Makefile (1)
298-298: LGTM: Host identity types package properly added to fast test suite.The addition of
./ee/server/service/hostidentity/typestoFAST_PKGS_TO_TESTcorrectly integrates the new host identity SCEP functionality into the fast testing pipeline.server/datastore/mysql/scep.go (2)
14-14: LGTM: Import change consistent with certificate encoding centralization.The replacement of the Apple MDM import with the generic certificate package follows the same refactoring pattern applied across the codebase.
93-93: LGTM: Function call correctly updated to use centralized encoding.The change from
apple_mdm.EncodeCertPEM(crt)tocertificate.EncodeCertPEM(crt)in thePutmethod maintains the same functionality while using the centralized certificate encoding utilities.server/mdm/apple/cert.go (1)
205-205: LGTM: Function delegation improves architectural separation.The delegation of SCEP CA certificate generation to
depot.NewSCEPCACertKey()is a well-structured architectural improvement that centralizes certificate generation logic in the appropriate package while maintaining the same function signature and behavior.server/fleet/mdm.go (1)
770-773: LGTM! Host identity CA asset constants are properly defined.The new constants follow the established naming pattern and are consistent with the existing MDM asset constants. They provide clear, descriptive names for the host identity root CA certificate and key assets.
server/service/testing_utils.go (3)
21-21: LGTM! Import statement is correct.The import of the hostidentity package is necessary for the new SCEP registration functionality.
367-367: LGTM! New test option field is properly integrated.The
HostIdentitySCEPStoragefield follows the established pattern for optional test features and uses the appropriate type.
475-477: LGTM! Conditional SCEP registration follows established patterns.The conditional registration logic is consistent with how other optional features are handled in this function. The nil check prevents potential panics, and the error handling using
require.NoErroris appropriate for test code.cmd/fleet/serve.go (2)
28-28: Import addition looks good.The import for the hostidentity package is correctly placed and follows the existing import organization pattern.
1197-1208: Host Identity SCEP integration is well-implemented.The implementation follows the established pattern for premium features:
- Properly checks for private key configuration
- Creates the depot with appropriate error handling
- Registers SCEP handlers conditionally
- Provides clear warning message when private key is missing
The error handling is consistent with other premium feature initializations in the codebase.
pkg/certificate/certificate.go (2)
6-6: Import additions are appropriate.The new imports for
crypto/rsaandencoding/pemare correctly added to support the new PEM encoding functions.Also applies to: 9-9
243-259: PEM encoding functions are well-implemented.Both functions follow standard PEM encoding patterns:
EncodeCertPEMcorrectly creates a PEM block with "CERTIFICATE" typeEncodePrivateKeyPVMproperly uses PKCS1 format for RSA private keys- Both functions use the appropriate
pem.EncodeToMemoryfor encodingThe implementation is clean and follows established PEM encoding conventions.
ee/server/service/hostidentity/config.go (1)
1-51: Asset initialization function is well-implemented.The
initAssetsfunction follows a solid pattern:
- Checks for existing assets first to avoid unnecessary generation
- Generates new SCEP CA certificate and key only when needed
- Uses the new generic PEM encoding functions from the certificate package
- Proper error handling with informative error messages
- Follows the established pattern for asset initialization in the codebase
The logic correctly handles the case where assets don't exist (first-time setup) and avoids regenerating existing assets.
server/service/mdm.go (2)
24-25: Import aligns with cross-package reuse – looks goodSwitching to the generic
pkg/certificatehelpers keeps the service layer agnostic of Apple-specific utilities. No issues.
2529-2531: Consistent use of new helpers – goodThe PEM encoding in asset persistence now routes through the shared
certificatepackage. Implementation stays functionally identical.server/datastore/mysql/host_identity_scep.go (1)
12-24: LGTM! Well-implemented certificate retrieval method.The implementation correctly:
- Uses parameterized queries to prevent SQL injection
- Includes proper validation checks for expiration (
not_valid_after > NOW()) and revocation (revoked = 0)- Follows the established datastore pattern with appropriate error handling
- Uses the reader connection for this read-only operation
server/datastore/mysql/mysql.go (3)
20-20: LGTM! Import added for host identity SCEP depot.
187-191: LGTM! Method renamed to reflect generic SCEP usage.The rename from
NewMDMAppleSCEPDepottoNewSCEPDepotbetter reflects the broader SCEP usage beyond Apple MDM.
193-197: LGTM! Host identity SCEP depot factory method properly implemented.The method correctly delegates to the hostidentity depot package and follows the established pattern for depot factory methods.
server/mdm/scep/depot/fleet.go (2)
11-33: LGTM! Secure and well-implemented SCEP CA certificate generation.The implementation correctly:
- Uses 2048-bit RSA keys as required by Apple
- Generates cryptographically secure random keys
- Creates a self-signed CA certificate with appropriate 10-year validity
- Includes proper error handling at each step
- Returns both the parsed certificate and private key
35-41: LGTM! Proper RSA key generation with Apple-compliant size.The 2048-bit key size is correctly enforced per Apple's requirements for CSR key sizes.
server/datastore/mysql/common_mysql/testing_utils/testing_utils.go (2)
98-98: LGTM! Useful addition for test isolation.The
UniqueTestNamefield allows explicit control over test database naming, improving test isolation and configurability.
187-201: LGTM! Well-implemented test name override logic.The implementation correctly:
- Uses explicit
UniqueTestNamewhen provided- Falls back to runtime-derived test names when not specified
- Maintains backward compatibility
- Preserves the existing name sanitization logic
ee/server/integrationtest/hostidscep/suite.go (1)
15-51: LGTM! Comprehensive integration test suite setup.The test suite properly:
- Embeds the existing
BaseSuitefor common functionality- Sets up premium license tier appropriate for host identity features
- Initializes the host identity SCEP depot with proper logging
- Configures the server with all necessary components
- Sets up authentication token for API testing
- Follows established integration test patterns
server/mock/datastore_mock.go (4)
14-14: LGTM: Import addition is correct.The import for hostidentity types is necessary for the
types.HostIdentityCertificatetype used in the new mock method.
1416-1416: LGTM: Function type definition follows established patterns.The function signature is well-designed with appropriate parameter and return types for certificate retrieval by serial number.
3504-3505: LGTM: Struct fields follow established mock patterns.The function field and invocation tracking field are consistent with the patterns used throughout the mock datastore.
8375-8380: LGTM: Method implementation follows standard mock patterns.The implementation correctly handles thread safety with mutex locking, tracks invocation, and delegates to the configured function. This is consistent with all other mock methods in the codebase.
ee/server/service/hostidentity/types/host_identity_certificates_test.go (1)
1-123: Comprehensive test coverage!The test suite thoroughly covers both positive and negative test cases for ECDSA public key handling, including supported curves (P256, P384), error scenarios (unsupported curves, invalid formats, empty data), and proper validation of the marshaling/unmarshaling process.
ee/server/service/hostidentity/types/host_identity_certificates.go (1)
1-68: Well-structured ECDSA key handling implementation!The implementation correctly handles ECDSA public keys for both P256 and P384 curves, with proper validation of the uncompressed point format (0x04 prefix) and appropriate coordinate extraction. The 100-byte limit for
PublicKeyRawappropriately accommodates P384 keys (97 bytes).ee/server/service/hostidentity/scep.go (1)
1-187: Well-implemented SCEP service with proper security controls!The implementation includes appropriate challenge-based enrollment validation, clear CA capability declarations, and proper error handling. The monitoring limitations are well-documented for future enhancement.
ee/server/integrationtest/hostidscep/hostidscep_test.go (8)
1-22: Well-structured imports and package declaration.The imports are appropriate for the cryptographic operations and testing framework. The package structure follows Go conventions for integration tests.
26-44: Excellent test organization with proper cleanup.The test structure follows best practices with:
- Table-driven tests for multiple scenarios
- Proper database cleanup between test runs
- Clear separation of success and failure cases
The truncation of both
host_identity_scep_serialsandhost_identity_scep_certificatestables ensures test isolation.
46-54: Good parametrization of ECC curve testing.Testing both P256 and P384 curves ensures compatibility with common elliptic curve standards. This approach allows easy addition of other curves if needed.
56-184: Comprehensive SCEP certificate enrollment test with thorough validation.This test covers the complete SCEP workflow:
- Enrollment secret setup
- ECC key generation
- CA certificate retrieval
- CSR creation and signing
- SCEP protocol envelope handling
- Certificate validation
- Database storage verification
The dual validation approach (SCEP response + database lookup) ensures data integrity throughout the process.
186-230: Comprehensive failure scenario testing.The test cases cover important edge cases:
- Authentication failures (empty/wrong password)
- Input validation (CN length limits)
- Algorithm restrictions (non-ECC rejection)
This ensures the SCEP service properly validates inputs and enforces security policies.
232-236: Well-defined test configuration structure.The
SCEPFailureConfigstruct provides a clean way to parameterize failure tests. The fields are appropriate for testing different failure scenarios.
238-344: Thorough failure testing with proper error validation.The failure test function properly validates that SCEP requests fail with the expected status. The test correctly expects
scep.FAILUREstatus and verifies that the request does not succeed.
208-213: Verify SCEP CommonName length enforcementI didn’t find any explicit 255-char limit check for the CommonName in the hostidentity SCEP implementation. The integration test in
• ee/server/integrationtest/hostidscep/hostidscep_test.go (testGetCertFailures “CN longer than 255 characters”)
assumes the server will reject a CN >255. Please confirm that the SCEP endpoint in ee/server/service/hostidentity (and its depot) enforces this limit—or add the check if it’s missing—to keep the test and implementation in sync.server/datastore/mysql/schema.sql (2)
521-525: Table definition looks solid.Primary key on
serial,AUTO_INCREMENTstarts at 2 to reserve the CA’s serial, charset and collation are consistent with the rest of the schema. No issues spotted.
1412-1414: No actionable feedback –AUTO_INCREMENT=401and the new migration record keep the sequence in sync.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ee/server/integrationtest/hostidentity/hostidscep_test.go (1)
56-158: Consider breaking down this function for better maintainability.While the test logic is comprehensive and correct, this 100+ line function could benefit from being broken down into smaller helper functions for improved readability and maintainability.
Consider extracting logical sections into helper functions:
-func testGetCertWithCurve(t *testing.T, s *Suite, curve elliptic.Curve) { - ctx := t.Context() - // Create an enrollment secret - err := s.DS.ApplyEnrollSecrets(ctx, nil, []*fleet.EnrollSecret{ - { - Secret: testEnrollmentSecret, - }, - }) - require.NoError(t, err) +func testGetCertWithCurve(t *testing.T, s *Suite, curve elliptic.Curve) { + ctx := t.Context() + + // Setup + setupEnrollmentSecret(t, ctx, s.DS) + eccPrivateKey := generateECCKey(t, curve) + scepClient, caCerts := setupSCEPClient(t, ctx, s) + + // Create and send SCEP request + cert := performSCEPEnrollment(t, ctx, scepClient, caCerts, eccPrivateKey, curve) + + // Verify certificate + verifyCertificateProperties(t, cert, eccPrivateKey, curve) + verifyStoredCertificate(t, ctx, s.DS, cert, eccPrivateKey, curve) +} + +func setupEnrollmentSecret(t *testing.T, ctx context.Context, ds fleet.Datastore) { + err := ds.ApplyEnrollSecrets(ctx, nil, []*fleet.EnrollSecret{ + {Secret: testEnrollmentSecret}, + }) + require.NoError(t, err) +} + +func generateECCKey(t *testing.T, curve elliptic.Curve) *ecdsa.PrivateKey { + eccPrivateKey, err := ecdsa.GenerateKey(curve, rand.Reader) + require.NoError(t, err) + return eccPrivateKey +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ee/server/integrationtest/hostidentity/hostidscep_test.go(1 hunks)ee/server/integrationtest/hostidentity/suite.go(1 hunks)ee/server/service/hostidentity/depot/depot.go(1 hunks)ee/server/service/hostidentity/scep.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ee/server/service/hostidentity/scep.go
- ee/server/service/hostidentity/depot/depot.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.721Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.566Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
ee/server/integrationtest/hostidentity/hostidscep_test.go (3)
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.566Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: ee/server/service/hostidentity/depot/depot.go:108-111
Timestamp: 2025-07-08T16:12:48.756Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the SCEP depot interface methods like Put() do not accept context parameters, and the common_mysql.WithRetryTxx callback function type TxFn only receives a transaction parameter, not a context. Therefore, using context.Background() in tx.ExecContext calls within the transaction callback is the correct approach.
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.721Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (32)
- GitHub Check: lint (ubuntu-latest)
- GitHub Check: lint (macos-latest)
- GitHub Check: lint (windows-latest)
- GitHub Check: test-go (main, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (service, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (scripts, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (main, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (service, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (fast, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go-nanomdm
- GitHub Check: check-doc-gen
- GitHub Check: build-binaries
- GitHub Check: test-preview (ubuntu-latest)
- GitHub Check: publish
- GitHub Check: test-db-changes
- GitHub Check: test-packaging (ubuntu-latest)
- GitHub Check: test-packaging (macos-15)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (go)
- GitHub Check: build-and-check
🔇 Additional comments (4)
ee/server/integrationtest/hostidentity/suite.go (1)
19-51: LGTM! Well-structured test suite setup.The test suite properly initializes all required components for host identity SCEP integration testing, including the MySQL datastore, Fleet service with premium license, and the SCEP depot.
ee/server/integrationtest/hostidentity/hostidscep_test.go (3)
26-44: LGTM! Proper test setup with table cleanup.The test structure ensures proper isolation between test runs by truncating the relevant tables.
160-189: LGTM! Well-implemented helper function.The function correctly creates the RSA key and certificate required for SCEP protocol envelope encryption.
191-235: LGTM! Comprehensive failure test coverage.The test cases properly cover important failure scenarios including authentication, input validation, and algorithm restrictions.
|
Merging as Victor is OOO. |
2 participants
Fixes #30458
Contributor docs PR: #30651
Checklist for submitter
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Documentation