[ca] add Private Server certificate support#18
Conversation
WalkthroughThis update changes the dependency version in the module file and refactors the certificate commands. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as CLI Application
participant CMD as Certificate Command
participant FACTORY as NewIPCertificateCommand
participant UTILS as Certificate Utils
participant CA as Certificate Authority
U->>CLI: Invoke certificate command (webhooks/private)
CLI->>FACTORY: Create command instance
FACTORY->>CMD: Execute command logic
CMD->>UTILS: Validate IP address and prepare CSR template
UTILS->>CA: Send certificate request (CSR)
CA-->>UTILS: Respond with certificate data
UTILS->>CMD: Return certificate and key
CMD->>CLI: Output certificate and key to user
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/commands/ca/webhooks.go (1)
49-50: Avoid relying solely on CommonName for IP addresses.While it’s acceptable to store the IP address in the CommonName, modern best practices strongly recommend using the SAN (Subject Alternative Name) field. Confirm that your clients verify SAN for IP-based certificates rather than relying on the CommonName, which is deprecated in many contexts.
internal/commands/ca/utils.go (2)
21-42: Consider removing unnecessary key usages if not required.The
nonRepudiationbit (0x05) may not be necessary for a typical server certificate. If it's not required for your use case, you can conserve clarity by removing it.Possible change:
- Value: []byte{0x03, 0x02, 0x05, 0xa0}, // nonRepudiation, digitalSignature, keyEncipherment + Value: []byte{0x03, 0x02, 0x07, 0xa0}, // only digitalSignature, keyEncipherment
44-107: Make the timeout configurable & consider encryption for private keys.
- The 30-second timeout may not be suitable for all environments. Exposing this as a configurable flag or parameter would help adapt to varied network conditions.
- Storing an unencrypted private key on disk can pose risks in certain environments. Consider passphrase-based encryption for an extra layer of security.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(1 hunks)internal/commands/ca/utils.go(1 hunks)internal/commands/ca/webhooks.go(1 hunks)
🔇 Additional comments (2)
go.mod (1)
6-6:Details
❓ Verification inconclusive
Confirm no breaking changes in the updated dependency.
Please ensure that the upgrade to
v1.5.5-0.20250309004612-ebfc247ebb13doesn't introduce any backward-incompatible changes or security vulnerabilities.Run this script to check for known advisories and confirm backward compatibility:
🏁 Script executed:
#!/bin/bash # Validate the updated version of github.com/android-sms-gateway/client-go echo "Checking for known security advisories in 'client-go'..." gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: GO, package: "github.com/android-sms-gateway/client-go") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 502
Dependency Upgrade Verification: No Known Security Vulnerabilities Detected
- The GitHub API query returned no security advisories for
github.com/android-sms-gateway/client-go v1.5.5-0.20250309004612-ebfc247ebb13.- While no automated tools flagged backward-incompatible changes, please manually review the dependency’s release notes or commit history to confirm that no breaking changes have been introduced.
internal/commands/ca/webhooks.go (1)
55-55: Looks good.Delegating to
requestCertificateis a clean approach that maintains modularity and readability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/commands/ca/utils.go (2)
21-42: Add tests fornewServerCertificateRequestPEM.This function is critical for generating the CSR and sets custom key usage and extended key usage extensions. However, no tests verify these extensions in the resulting PEM. Consider adding unit tests to ensure correctness (including verifying whether
nonRepudiationis indeed required for server certificates).Would you like me to draft basic unit tests or open an issue for test coverage?
44-108: Make the certificate polling timeout configurable.The 30-second timeout may be too short or too long, depending on the CA’s response time. Consider allowing a user-configurable timeout to accommodate slower or faster certificate issuance.
internal/commands/ca/private.go (1)
49-54: Consider supporting additional SAN entries.Currently, the CSR only includes the single private IP address in the SAN field. If you foresee cases where multiple IP addresses or hostnames might be needed, consider extending this logic to include them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
internal/commands/ca/ca.go(1 hunks)internal/commands/ca/private.go(1 hunks)internal/commands/ca/utils.go(1 hunks)internal/commands/ca/webhooks.go(1 hunks)
🔇 Additional comments (4)
internal/commands/ca/ca.go (1)
9-9: Addition of theprivatecommand looks good.No issues found. This neatly extends the existing CLI commands array.
internal/commands/ca/private.go (1)
45-47: Confirm the scope ofIsPrivateusage.While
netipAddr.IsPrivate()correctly checks for RFC1918 addresses (and certain IPv6 ranges), it excludes other non-public address types such as link-local addresses. Ensure this aligns with your intended usage constraints.Would you like to conduct additional validation steps for addresses outside standard private blocks?
internal/commands/ca/webhooks.go (2)
49-54: LGTM: Clean structure for the CSR templateThe CSR template structure is well-formed with the subject containing the IP address as the common name and properly including the IP address in the CSR.
56-56:Details
✅ Verification successful
LGTM: Good refactoring to abstract certificate request logic
Excellent refactoring! Moving the certificate generation logic to a shared
requestCertificatefunction improves code maintainability and reusability. This follows the DRY principle and makes the code more modular.To complete my review, it would be helpful to verify that the
requestCertificatefunction properly handles all the functionality that was previously inline:
🏁 Script executed:
#!/bin/bash # Check implementation of the requestCertificate function grep -r "func requestCertificate" internal/commands/ca/Length of output: 185
LGTM: Well-executed abstraction of certificate request logic
Excellent refactoring—the certificate generation is now centralized in the shared
requestCertificatefunction (located ininternal/commands/ca/utils.go), which improves both maintainability and reusability. The grep output confirms the presence of therequestCertificatefunction, ensuring that its encapsulated logic now handles the functionality that was previously inline.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/commands/ca/common/command.go (1)
14-35: Add a doc comment for clarity.
This is a public function crucial to your CLI’scaworkflow. Adding a brief doc comment explaining its purpose (e.g., “Creates a CLI command to issue a certificate for private IP addresses, ensuring it meets CA requirements.”) will aid maintainability and readability.internal/commands/ca/common/utils.go (1)
44-108: Consider allowing adjustable timeouts or improved logging.
Waiting up to 30 seconds in a loop is a valid approach, but users operating in slower or more network-constrained environments may need more time. You could parameterize the timeout duration and log more details (e.g., the number of retries) to enhance the user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/commands/ca/common/command.go(1 hunks)internal/commands/ca/common/utils.go(1 hunks)internal/commands/ca/private.go(1 hunks)internal/commands/ca/webhooks.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/commands/ca/private.go
🔇 Additional comments (2)
internal/commands/ca/common/command.go (1)
36-58:Details
❓ Verification inconclusive
Confirm netip private range coverage.
WhilenetipAddr.IsPrivate()typically covers RFC1918 and IPv6 unique local addresses, some loopbacks or other special addresses might not be treated the same way. Verify that the desired addresses (e.g., 127.0.0.1) behave as intended.
🌐 Web query:
Does netip.IsPrivate() include loopback addresses, such as 127.0.0.1 or ::1?💡 Result:
The
netip.IsPrivate()method in Go does not include loopback addresses like127.0.0.1(IPv4) or::1(IPv6). Here's why:Key Distinctions in Go’s Implementation
Loopback vs. Private Ranges
- Loopback addresses (
127.0.0.1/::1) are classified separately viaIsLoopback()- Private addresses (
10.0.0.0/8,172.16.0.0/12,192.168.0.0/16for IPv4;fc00::/7for IPv6) are handled byIsPrivate()Code Behavior
127.0.0.1returns:
IsLoopback() = trueIsPrivate() = false[1][2][7][11]::1returns:
IsLoopback() = trueIsPrivate() = false[1][2][11]Language Comparison Insight
While some languages (like Node.js and PHP) classify loopback addresses as private, Go follows RFC specifications strictly:
- RFC 1918 (IPv4 private ranges)
- RFC 4193 (IPv6 unique local addresses)
- RFC 5735 (loopback as separate category) [2][6][16]
Practical Example
package main import ( "fmt" "net/netip" ) func main() { ipv4Loopback := netip.MustParseAddr("127.0.0.1") ipv6Loopback := netip.MustParseAddr("::1") fmt.Println("IPv4 Loopback IsPrivate:", ipv4Loopback.IsPrivate()) // false fmt.Println("IPv6 Loopback IsPrivate:", ipv6Loopback.IsPrivate()) // false }This design ensures clear separation between network-local communication (loopback) and organization-internal addressing (private ranges).
Citations:
- 1: https://github.com/golang/go/blob/master/src/net/netip/netip.go
- 2: https://infosecwriteups.com/understanding-inconsistencies-in-ip-address-classification-across-programming-languages-19b2645b10a9
- 3: https://dev.to/leapcell/understanding-network-interfaces-loopback-local-ips-and-public-ips-2p47
- 4: https://go.dev/src/net/ip.go
- 5: https://docs.python.org/3/library/ipaddress.html
- 6: https://www.hostwinds.com/blog/loopback-address
- 7: https://github.com/golang/go/blob/master/src/net/ip.go
- 8: http://www.tcpipguide.com/free/t_IPReservedPrivateandLoopbackAddresses-3.htm
- 9: https://learn.microsoft.com/en-us/dotnet/api/system.net.ipaddress.loopback?view=net-9.0
- 10: https://learn.microsoft.com/en-us/dotnet/api/system.net.ipaddress.ipv6loopback?view=net-9.0
- 11: https://android.googlesource.com/platform/prebuilts/go/linux-x86/+/refs/heads/android-t-preview-1/src/net/netip/netip.go
- 12: https://tip.golang.org/pkg/net/?m=old
- 13: https://pkg.go.dev/net/netip
- 14: https://tip.golang.org/src/net/netip/netip.go
- 15: https://metacpan.org/pod/Net::IP
- 16: https://dev.to/rezmoss/deep-dive-into-netnetip-addr-methods-57-2bh
- 17: https://learn.microsoft.com/en-us/dotnet/api/system.net.ipaddress.isloopback?view=net-9.0
- 18: https://dev.to/rezmoss/understanding-gos-netnetip-addr-type-a-deep-dive-17-j7b
Attention: Verify IP Address Acceptance Criteria
The current validation usesnetipAddr.IsPrivate()to ensure the provided IP address falls within the designated private ranges (RFC1918 for IPv4 and RFC4193 for IPv6). Note that according to Go’s implementation, loopback addresses (e.g., 127.0.0.1 and ::1) are not considered private (they are identified viaIsLoopback()).
- Please confirm that rejecting loopback addresses is intentional for this certificate request functionality.
- If loopback addresses should be treated as valid input, consider updating the IP validation logic to also allow addresses for which
netipAddr.IsLoopback()returns true.internal/commands/ca/webhooks.go (1)
8-13: Refactor looks great.
Delegating the certificate logic tocommon.NewIPCertificateCommandsimplifies thewebhookscommand and helps reduce duplicate code.
| func newServerCertificateRequestPEM(template x509.CertificateRequest, priv *ecdsa.PrivateKey) ([]byte, error) { | ||
| template.SignatureAlgorithm = x509.ECDSAWithSHA256 | ||
| template.ExtraExtensions = []pkix.Extension{ | ||
| { | ||
| Id: []int{2, 5, 29, 15}, // keyUsage OID | ||
| Critical: true, | ||
| Value: []byte{0x03, 0x02, 0x05, 0xa0}, // nonRepudiation, digitalSignature, keyEncipherment | ||
| }, | ||
| { | ||
| Id: []int{2, 5, 29, 37}, // extendedKeyUsage OID | ||
| Critical: false, | ||
| Value: []byte{0x30, 0x06, 0x06, 0x04, 0x2b, 0x06, 0x01, 0x05, 0x05, 0x07, 0x03, 0x01}, // serverAuth | ||
| }, | ||
| } | ||
|
|
||
| csrBytes, err := x509.CreateCertificateRequest(rand.Reader, &template, priv) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create certificate request: %w", err) | ||
| } | ||
|
|
||
| return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csrBytes}), nil | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Double-check extension OIDs and usage bits.
The code hard-codes OIDs for key usage (nonRepudiation, digitalSignature, keyEncipherment) and extended key usage (serverAuth). Please confirm with your CA requirements that these properties align with your intended usage.
🌐 Web query:
Are these OIDs (2.5.29.15, 2.5.29.37) and usage bits correct for typical serverAuth certificates in x509?
💡 Result:
The OIDs 2.5.29.15 (Key Usage) and 2.5.29.37 (Extended Key Usage) are critical components of X.509 certificates, particularly for server authentication (serverAuth). Here's how they align with typical serverAuth certificate requirements:
Key Usage (2.5.29.15)
- Purpose: Defines cryptographic operations permitted for the public key.
- Typical Bits for ServerAuth:
- Digital Signature: Required for TLS handshake authentication[5][8].
- Key Encipherment: Necessary for RSA-based key exchange mechanisms[5][8][16].
- Criticality: Often marked critical to enforce strict usage[3][6].
- OID Structure: Defined in RFC 3280 and PKIX standards[10].
Extended Key Usage (2.5.29.37)
- Purpose: Specifies application-specific uses. For serverAuth:
- ServerAuth OID:
1.3.6.1.5.5.7.3.1(mandatory for TLS server certificates)[6][11].
- ServerAuth OID:
- Criticality: Can be critical or non-critical. Let’s Encrypt marks it non-critical[8], while strict implementations may require critical marking[11].
- Example Configuration:
extendedKeyUsage = serverAuth, clientAuth # Common in multi-purpose certs[5][8]
Validation Requirements
- BasicConstraints: Must be
CA:FALSEfor end-entity certificates[5][9]. - Subject Alternative Name (SAN): Required for modern certificates[8][16].
- Key Usage Compatibility:
- ServerAuth requires alignment between
keyUsageandextendedKeyUsage[6][10]. - Mismatches (e.g., missing
keyEnciphermentfor RSA) cause validation failures[5][11].
- ServerAuth requires alignment between
Common Pitfalls
- Missing EKU: Certificates without
id-kp-serverAuthin EKU are rejected by strict clients (e.g., Samsung S8 in SMTP/TLS)[8]. - Overly Broad Key Usage: Including unnecessary bits (e.g.,
dataEncipherment) may reduce security[8].
Example Configuration (OpenSSL)
[usr_cert]
keyUsage = digitalSignature, keyEncipherment
extendedKeyUsage = serverAuth
basicConstraints = CA:FALSE
subjectAltName = DNS:example.comThis configuration ensures compliance with RFC 5280 and PKIX guidelines[5][6][10].
Citations:
- 1: https://docs.redhat.com/en/documentation/red_hat_certificate_system/9/html/administration_guide/standard_x.509_v3_certificate_extensions
- 2: https://pki-tutorial.readthedocs.io/en/stable/extensions.html
- 3: https://www.alvestrand.no/objectid/2.5.29.15.html
- 4: https://discuss.hashicorp.com/t/usage-of-certificate-oids-in-policies-when-using-cert-auth/31084
- 5: https://access.redhat.com/solutions/28965
- 6: https://www.alvestrand.no/objectid/2.5.29.37.html
- 7: https://en.wikipedia.org/wiki/X.509
- 8: https://community.letsencrypt.org/t/x509v3-extensions-for-mtas/37014
- 9: https://docs.oracle.com/cd/E19957-01/816-5539-10/app_ext.htm
- 10: https://datatracker.ietf.org/doc/html/rfc5280
- 11: https://discussions.apple.com/thread/255182051
- 12: https://cryptography.io/en/latest/x509/reference/
- 13: https://www.encryptionconsulting.com/education-center/what-is-an-oid/
- 14: https://cryptography.io/en/stable/x509/reference/
- 15: https://forums.oracle.com/ords/apexds/post/adding-a-user-notice-into-an-x-509-certificate-6655
- 16: https://www.gnutls.org/manual/html_node/certtool-Invocation.html
- 17: https://docs.oracle.com/javame/config/cdc/ref-impl/fp1.1.2/jsr219/java/security/cert/X509Certificate.html
- 18: https://www.bsi.bund.de/SharedDocs/Downloads/EN/BSI/CPT/CPT-Test-Tool-Specification_v1_1.pdf?__blob=publicationFile&v=2
- 19: Obtaining certificate purpose pyca/cryptography#6350
Action Required: Update DER Encoding for Extensions
Based on the verification:
- The keyUsage DER (OID 2.5.29.15) currently encodes a BIT STRING (
0x03, 0x02, 0x05, 0xa0) that sets only digitalSignature and keyEncipherment. If your CA requirements call for nonRepudiation as well, the bit string must be adjusted (for example, using a value that encodes all three bits, such as0x03,0x02,0x05,0xe0). - The extendedKeyUsage DER (OID 2.5.29.37) value appears to be mis-encoded. Typically for a serverAuth certificate the DER should be similar to:
0x30, 0x0a, 0x06, 0x08, 0x2b, 0x06, 0x01, 0x05, 0x05, 0x07, 0x03, 0x01
rather than the current0x30, 0x06, 0x06, 0x04, 0x2b, 0x06, 0x01, 0x05, 0x05, 0x07, 0x03, 0x01.
Please confirm these encodings match your certificate authority's requirements and update the code accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/commands/ca/common/command.go (3)
14-16: Consider enhancing the function documentation.While the current documentation provides a basic description of the function, it would be beneficial to include details about the parameters (especially what
typrepresents) and expected behavior of the function.// NewIPCertificateCommand creates a new CLI command for generating an IP certificate // of the specified type +// Parameters: +// - name: The name of the command +// - usage: Usage description displayed in help +// - aliases: Alternative names for the command +// - typ: Certificate signing request type (e.g., webhook, private server) +// Returns a configured CLI command that handles certificate generation
22-36: Consider using more descriptive flag names.While the current flag names are concise, more descriptive names like
--cert-outand--key-outmight improve clarity for users.&cli.StringFlag{ - Name: "out", + Name: "cert-out", Usage: "Certificate output file", Required: false, Value: "server.crt", }, &cli.StringFlag{ - Name: "keyout", + Name: "key-out", Usage: "Private key output file", Required: false, Value: "server.key", },
16-62: Consider dynamic default output filenames.The current default filenames ("server.crt" and "server.key") are generic. Consider incorporating the IP address or a timestamp in the default filenames to make them unique for each certificate generation.
&cli.StringFlag{ Name: "out", Usage: "Certificate output file", Required: false, - Value: "server.crt", + Value: "", }, &cli.StringFlag{ Name: "keyout", Usage: "Private key output file", Required: false, - Value: "server.key", + Value: "", },And add this logic to the Action function:
Action: func(c *cli.Context) error { ip := c.Args().Get(0) if ip == "" { return cli.Exit("IP address is empty", codes.ParamsError) } netipAddr, err := netip.ParseAddr(ip) if err != nil { return cli.Exit(err.Error(), codes.ParamsError) } if !netipAddr.IsPrivate() { return cli.Exit("IP address is not private", codes.ParamsError) } + // Set default filenames if not provided + certOut := c.String("out") + if certOut == "" { + certOut = fmt.Sprintf("%s.crt", netipAddr.String()) + } + + keyOut := c.String("keyout") + if keyOut == "" { + keyOut = fmt.Sprintf("%s.key", netipAddr.String()) + } + + // Update context with new values + c.Set("out", certOut) + c.Set("keyout", keyOut) csrTemplate := x509.CertificateRequest{ Subject: pkix.Name{ CommonName: netipAddr.String(), }, IPAddresses: []net.IP{netipAddr.AsSlice()}, } return requestCertificate(c, typ, csrTemplate) },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/commands/ca/common/command.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
internal/commands/ca/common/command.go (4)
1-12: Well-organized import structure.The package declaration and imports are clean and properly organized, with a clear separation between standard library packages and project-specific imports.
43-50: Good IP validation approach.The code properly validates that the provided IP address is valid and private, which is essential for security. Using the
netippackage provides robust IP address parsing and validation.
52-58: Well-structured CSR template creation.The certificate signing request template is properly created with the IP address set as both the subject common name and in the IP addresses field.
59-59:Details
✅ Verification successful
Verify the implementation of the requestCertificate function.
The code calls a
requestCertificatefunction that isn't defined in this file. Ensure this function is properly implemented and accessible.
🏁 Script executed:
#!/bin/bash # Let's find the implementation of requestCertificate function grep -r "func requestCertificate" --include="*.go" ./internal/commands/ca/Length of output: 213
Implementation of requestCertificate Verified in utils.go
The
requestCertificatefunction is implemented in./internal/commands/ca/common/utils.goas confirmed by the search output. No further action is required on this part.
452f99a to
ddf7efe
Compare
Summary by CodeRabbit