Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix private key parsing and add fallback on ACME #827

Merged
merged 3 commits into from
Oct 16, 2024

Conversation

paulgb
Copy link
Member

@paulgb paulgb commented Oct 16, 2024

As part of the version updates we evidently lost the ability to parse pem files that have the heading PRIVATE KEY and instead need to specifically say RSA PRIVATE KEY.

Also, if we do fail, we should just fall back on ACME.

@paulgb paulgb requested a review from rolyatmax October 16, 2024 18:53
Copy link

vercel bot commented Oct 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
plane ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 16, 2024 7:08pm

Copy link
Contributor

coderabbitai bot commented Oct 16, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The changes in the pull request encompass multiple files, primarily focusing on error handling and test structure. In the cert_manager.rs file, the CertManager::new method has been modified to improve error handling during certificate loading. The previous direct loading approach has been replaced with a match statement that logs errors and sets initial_cert to None in case of failure, enhancing robustness without altering the overall logic or public interface.

In the cert_manager.rs test file, the cert_manager_does_refresh_eab function has been updated to include a timeout parameter for the test execution. The structure of the code has been refined for better organization, with variable initializations encapsulated within blocks and the introduction of an eab_keypair. Additionally, a new block has been added to create a second cert_watcher, which loads certificates from disk, thereby introducing a new flow in the test.

Lastly, in the cert_pair.rs file, the label for the PEM encoding of the private key has been changed from "PRIVATE KEY" to "RSA PRIVATE KEY", clarifying the key type in the output format. Overall, these changes enhance error handling, test organization, and output clarity without affecting public method signatures.


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
plane/src/proxy/cert_manager.rs (2)

141-151: Improved error handling for certificate loading

The changes enhance the robustness of the certificate loading process by providing more detailed error logging and allowing graceful fallback to ACME when loading fails. This aligns well with the PR objective of falling back on ACME for cert provisioning.

Consider adding a debug log statement when successfully loading the certificate to provide a complete picture of the process. For example:

Ok(cert) => {
    tracing::debug!(?cert_path, "Successfully loaded certificate from file");
    Some(cert)
},

This addition would help in troubleshooting and provide a clear indication of successful certificate loading from a file.


Line range hint 1-624: Suggestions for future improvements

While the current changes are appropriate, here are some suggestions for future enhancements to improve the overall code quality:

  1. Consider enhancing error handling in the refresh_loop function. Currently, errors are logged but the loop continues. Depending on the nature of the error, it might be beneficial to implement a backoff strategy or exit the loop in certain scenarios.

  2. The get_certificate function is quite long and handles multiple responsibilities. Consider breaking it down into smaller, more focused functions to improve readability and maintainability. For example, you could create separate functions for handling authorizations, challenges, and order finalization.

  3. Replace magic numbers with named constants. For instance, in gen_rsa_private_key(4096), consider defining a constant like const RSA_KEY_SIZE: u32 = 4096; and using it instead.

These suggestions are not critical for the current PR but could be considered for future refactoring to improve the overall code structure and maintainability.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 77698c2 and 97fe635.

📒 Files selected for processing (1)
  • plane/src/proxy/cert_manager.rs (1 hunks)
🧰 Additional context used

@paulgb paulgb changed the title Fall back on ACME for cert provisioning Fix private key parsing and add fallback on ACME Oct 16, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
plane/plane-tests/tests/cert_manager.rs (1)

70-74: Approved with suggestion: EAB configuration added

The addition of External Account Binding (EAB) configuration is a valuable enhancement to the test. It aligns well with the test function's purpose.

However, consider replacing the hardcoded EAB values with constants or environment variables for better maintainability and security.

Example improvement:

const EAB_KID: &str = "kid-1";
const EAB_HMAC_KEY: &str = "zWNDZM6eQGHWpSRTPal5eIUYFTu7EajVIoguysqZ9wG44nMEtx3MUAsUDkMTQ12W";

let eab_keypair = AcmeEabConfiguration::new(
    EAB_KID.to_string(),
    EAB_HMAC_KEY.to_string(),
)
.unwrap();

Also applies to: 76-84

plane/src/proxy/cert_pair.rs (1)

Line range hint 124-143: Enhance security and error handling in the save method

While the current implementation of the save method is functional, there are a few areas where it could be improved:

  1. Security:

    • The method sets correct file permissions (0o600) only for new files. Consider updating permissions for existing files as well to ensure consistent security.
  2. Error handling:

    • While using Result is good, the error messages could be more specific to aid in debugging.

Here's a suggested improvement:

pub fn save(&self, path: &Path) -> Result<()> {
    let cert_ders: Vec<&[u8]> = self.certified_key.cert.iter().map(|cert| cert.as_ref()).collect();
    let cert = pem::encode_many(
        cert_ders.into_iter().map(|cert_der| Pem::new("CERTIFICATE", cert_der)).collect::<Vec<_>>().as_slice(),
    );

    let key = pem::encode(&Pem::new("RSA PRIVATE KEY", self.private_key_der.clone()));

    let cert_pair = SerializedCertificatePair { cert, key };

    let contents = serde_json::to_string_pretty(&cert_pair)
        .map_err(|e| anyhow!("Failed to serialize certificate pair: {}", e))?;

    // Create or open the file with specific permissions
    let mut file = std::fs::OpenOptions::new()
        .write(true)
        .create(true)
        .truncate(true)
        .mode(0o600)
        .open(path)
        .map_err(|e| anyhow!("Failed to create or open file: {}", e))?;

    // Write contents
    std::io::Write::write_all(&mut file, contents.as_bytes())
        .map_err(|e| anyhow!("Failed to write to file: {}", e))?;

    // Ensure file permissions are set correctly (in case the file already existed)
    let permissions = Permissions::from_mode(0o600);
    std::fs::set_permissions(path, permissions)
        .map_err(|e| anyhow!("Failed to set file permissions: {}", e))?;

    Ok(())
}

This version:

  1. Uses OpenOptions to create or open the file with the correct permissions.
  2. Ensures permissions are set correctly even for existing files.
  3. Provides more detailed error messages using anyhow!.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 97fe635 and 4694976.

📒 Files selected for processing (2)
  • plane/plane-tests/tests/cert_manager.rs (1 hunks)
  • plane/src/proxy/cert_pair.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
plane/plane-tests/tests/cert_manager.rs (5)

61-61: Approved: Increased test timeout

The change from #[plane_test] to #[plane_test(120)] increases the test timeout to 120 seconds. This is a reasonable adjustment given the complexity of the test and the potential time required for certificate operations.


63-65: Approved: Improved code structure and scoping

The restructuring of the test function enhances readability and organization. The earlier introduction of certs_dir and the use of block scoping for the main test body are good practices that improve code clarity and variable management.

Also applies to: 111-112


88-110: Approved: Robust certificate watcher and manager setup

The setup process for the certificate watcher and manager is well-structured and includes appropriate error handling. The use of with_timeout(60) for waiting for the initial certificate is a good practice to prevent the test from hanging. The ProxyConnection creation, while not directly used, appears to be part of the test setup to simulate a real-world scenario.


113-128: Approved: Added test for certificate loading without ACME config

This new block effectively tests the system's ability to load certificates from disk when no ACME configuration is provided. It's a valuable addition that ensures the fallback mechanism works correctly. The consistent use of with_timeout(60) maintains the robustness of the test.

This change enhances the test coverage by verifying both the ACME-based certificate provisioning and the fallback to disk-based loading.


61-128: Summary: Comprehensive improvements to certificate management testing

The changes in this PR significantly enhance the cert_manager_does_refresh_eab test function. Key improvements include:

  1. Increased test timeout for better handling of asynchronous operations.
  2. Improved code structure and scoping for better readability and maintainability.
  3. Addition of External Account Binding (EAB) configuration to test ACME cert provisioning.
  4. Implementation of a fallback scenario to test loading certificates from disk without ACME config.

These changes align well with the PR objective of falling back on ACME for cert provisioning and provide more comprehensive test coverage for the certificate management system.

plane/src/proxy/cert_pair.rs (1)

127-127: Verify compatibility and consider using PKCS#8 format

The change from "PRIVATE KEY" to "RSA PRIVATE KEY" makes the PEM label more specific to RSA keys. While this follows the PKCS#1 format for RSA keys, it might have some implications:

  1. It could potentially break compatibility with systems expecting the generic "PRIVATE KEY" label (PKCS#8 format).
  2. If the private key is not actually an RSA key, this label would be incorrect.

Consider the following suggestions:

  1. Verify that all systems interacting with these PEM files can handle the "RSA PRIVATE KEY" label.
  2. If possible, use the more modern PKCS#8 format with the generic "PRIVATE KEY" label, which can handle multiple key types.
  3. If you must use the RSA-specific format, add a check to ensure the key is actually an RSA key before encoding.

To ensure this change doesn't break existing functionality, please run the following script:

This script will help identify potential compatibility issues and areas that might need adjustment due to this change.

✅ Verification successful

Change to "RSA PRIVATE KEY" Verified

The update to use the "RSA PRIVATE KEY" label is consistent with the current implementation and does not introduce any compatibility issues within the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for usage of PEM decoding and potential compatibility issues

# Test 1: Search for PEM decoding in the codebase
echo "Searching for PEM decoding usage:"
rg --type rust 'pem::parse|rustls_pemfile::read_one|openssl::pkey::Private|load_key_der'

# Test 2: Check if there are any hardcoded expectations of "PRIVATE KEY" label
echo "Checking for hardcoded 'PRIVATE KEY' expectations:"
rg --type rust '"PRIVATE KEY"'

# Test 3: Look for any RSA-specific key handling
echo "Checking for RSA-specific key handling:"
rg --type rust 'RSA|Rsa'

Length of output: 714

@paulgb paulgb merged commit ef3fa13 into main Oct 16, 2024
7 checks passed
@paulgb paulgb deleted the paulgb/fallback-on-acme branch October 16, 2024 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants