Refactor and extend S/MIME signing support #406
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Moved all dummy certificate and key files along with their licenses to the testdata directory for better organization.
Corrected "S/MIME singed messages" to "S/MIME message signing" for accuracy. Also added clarification that this feature is experimental and currently supports only single-part messages.
Updated naming of S/MIME-related constants and types to adhere to Go naming conventions, improving clarity and maintaining uniformity across the codebase. Adjusted relevant tests and method calls to reflect these changes.
Revised test code to remove direct stdout usage in favor of buffers for better test isolation. Updated license headers with accurate attributions and documented the project's fork history for clarity.
Replaced the unused OIDDigestAlg parameter with a blank identifier (_) to improve clarity and indicate that it is not utilized. This change avoids misleading readers and aligns the function implementation with standard Go practices.
Renamed methods and variables to standardize S/MIME capitalization and naming, including `SignWithSMimeRSA` to `SignWithSMIMERSA` and `sMime` to `sMIME`. Additionally, fixed minor typos in error messages for improved clarity and updated comments for readability.
The util_test.go file has been removed as it is no longer required. The file contained test functions for loading cryptographic materials that are not used or relevant in the current codebase. This cleanup reduces unnecessary code and improves maintainability.
Unified the naming convention by renaming `SMime` to `SMIME` for improved consistency and readability. Updated all associated references in methods, comments, and test cases. Added helper functions to load dummy RSA and ECDSA cryptographic materials for testing purposes.
We don't want to export the PKCS7 code 3rd parties, therefore moved the PKCS7 implementation to an internal module for better encapsulation and renamed functions to use exportable naming conventions. Updated all references to reflect these changes across the codebase, ensuring consistent usability and improved clarity.
The redundant else block was removed to simplify the code. This change improves readability and adheres to cleaner coding practices. No functional behavior has been altered.
Renamed methods and related references from "WithSMimeSinging" to "WithSMIMESigning" for correctness and consistency. Updated associated test cases, comments, and documentation to reflect the renamed methods accurately.
Renamed parameter for clarity and adjusted logic to set `NoEncoding` when S/MIME signing is enabled. Consolidated nested if-statements for the encoding conditions by using the previously used `switch` statement, improving readability and maintainability.
Updated the function parameter name and associated comments to improve readability and consistency. This change ensures the naming aligns with the parameter's purpose and standard conventions.
Simplified the logic for building the content type string using `fmt.Sprintf`, enhancing readability and maintainability. Adjusted the condition to handle S/MIME-signed cases as the exception instead of the default.
This commit consolidates and refactors S/MIME-related methods in the `Msg` struct, improving clarity and consistency. Function and type names are updated for proper naming conventions, such as renaming `hasSMime` to `hasSMIME` and `MIMESMIME` to `MIMESMIMESigned`. Redundant code is reorganized or updated for better maintainability.
Updated error messages in test cases to provide clearer diagnostics and replaced `t.Errorf` with `t.Fatalf` where test execution must stop. Also, added a nil check for signature parts to prevent potential nil pointer dereferences.
Correct variable naming for keyPairTLS, handle nil input, and improve intermediate certificate parsing. Avoid duplicate signing in signMessage and track signing state with a new flag. These changes enhance clarity, robustness, and prevent redundant operations.
Simplified methods in S/MIME by removing redundant line parsing and restructuring encoding logic. Changed pointer-based returns to value-based in key methods for clarity and reliability. Cleaned up test cases to align with the updated code structure.
Consolidated RSA and ECDSA S/MIME signing into a single method to simplify keypair handling. Replaced custom key holder structure with `crypto.PrivateKey` for flexibility. Enhanced error messages and pre-checks for better diagnostics and maintainability.
Updated S/MIME signing functions to replace specific methods (`SignWithSMIMERSA` and `SignWithSMIMEECDSA`) with a unified `SignWithKeypair` method. Simplified test handling and refactored private key checks to make the codebase cleaner and reduce redundancy. Transitioned crypto material loaders to return generic `crypto.PrivateKey` for improved flexibility.
This change eliminates the `newSMIMEWithECDSA` function and associated imports, streamlining the codebase by removing unused ECDSA-related implementation. The update simplifies maintenance and focuses the library on core S/MIME functionality.
Updated the parameter name from `intermediateCertificate` to `intermediateCert` for improved readability and consistency. This change ensures better alignment with naming conventions and enhances code maintainability.
Refactored field names in the S/MIME struct for consistency and clarity (e.g., `intermediateCertificate` to `intermediateCert`). Updated function comments to improve detail and readability, ensuring better guidance for developers. Adjusted tests to align with renamed fields.
Merged redundant SMIME test cases and grouped them into structured subtests. Improved error handling and checks for various configurations, such as missing keys or certificates, while maintaining compatibility with Go 1.23 changes. This enhances readability, reduces duplication, and ensures comprehensive validation.
Move `getLeafCertificate` from `msg.go` to `smime.go` for better organization and reusability. Add new test cases to validate SMIME signing edge cases, including nil or malformed TLS certificates and unsupported private keys.
Unused constants related to broken RSA certificates were removed to simplify the code. This cleanup enhances code readability and maintains alignment with the test's actual requirements.
Swapped the order of "Content-Type" and "Content-Transfer-Encoding" headers in test cases to align with expected structure. This ensures the tests reflect the accurate header sequence for better reliability.
This commit removes the `GetOnlySigner` and `UnmarshalSignedAttribute` methods from the PKCS7 struct as they are no longer used. This cleanup simplifies the codebase by eliminating unnecessary functionality.
Refactored the S/MIME signing process to streamline logic, improve maintainability, and reduce code duplication. There was a general logic flaw in the whole SMIME signing code. It would only work with quoted-printable and raw encodings, while base64 and others would fail. Instead of forcing to render the message body into QP, we no perform a pre-signing render run of the whole mail which will output all message parts in proper formatting and encoding. The result of this pre-sign run is then used for the signature. It currently doesn't work with multipart messages yet, but we now make use of all available go-mail tools to get the message parts. This makes a bunch of methods in the smime.go obsolete. Removed unused and redundant methods, added safeguards for in-progress signing, and improved boundary generation for multipart messages. Updated existing tests to align with the new implementation.
Add `headerCount` to track header lines for accurate parsing and signing. Introduced boundary setting and refined message handling to ensure correct header offsets. Included debug logs for unsigned message data to aid troubleshooting.
Removed redundant flags and improved handling of S/MIME state during message signing to streamline the process and avoid duplicate signatures. Added `headerCount` to accurately determine the boundary between headers and body. Updated multipart handling to respect S/MIME signatures and fixed message boundary consistency. We are now able to S/MIME sign all kind of messages, including multipart messages and attachments.
Deleted `getCertFromCertsByIssuerAndSerial` and `unmarshalAttribute` as they were no longer used in the codebase. This reduces unnecessary code and improves maintainability.
The isCertMatchForIssuerAndSerial function was no longer referenced or used in the codebase. Removing it helps reduce dead code, improving maintainability and clarity. No functional changes were introduced.
Refactored the S/MIME logic by removing unused `IsSMIMESigned` method, redundant parameters, and unnecessary checks. Updated relevant calls in `msgwriter` and tests to streamline code and improve maintainability.
Updated the SignWithKeypair function signature for improved formatting consistency and refactored the variable declaration in a loop to use shorthand syntax. These changes enhance code readability and adhere to Go's best practices.
Replaced algorithm-specific logic with a more general crypto.Signer implementation for signing. Removed the unused ErrUnsupportedAlgorithm error and simplified the PKCS7 signing process for better maintainability and flexibility. This allows for ECDSA support.
Add new test cases for S/MIME signing validation while removing outdated tests. Improve code structure for handling invalid signing scenarios.
Refined the wording for S/MIME message signing to make it more concise and aligned with the existing items. This ensures clearer communication while maintaining consistency in the feature list.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #406 +/- ##
==========================================
+ Coverage 92.26% 94.67% +2.41%
==========================================
Files 30 30
Lines 3595 3515 -80
==========================================
+ Hits 3317 3328 +11
+ Misses 211 138 -73
+ Partials 67 49 -18 ☔ View full report in Codecov by Sentry. |
wneessen
changed the title
This commit ensures that errors from `randomBoundary()` are handled properly in `msg.go`, preventing potential failures during boundary assignment. A new test case for signing with a broken `rand.Reader` is added in `msg_test.go` to verify the behavior. Minor test descriptions were also updated for clarity.
Refactors and enhances the `randomBoundary` function to include proper error handling and uses it across the codebase. Adds accompanying tests to validate correct behavior and simulate error scenarios with a broken random reader.
Replaced `align` attribute with `style` for semantic HTML and added `alt` text to contributor image for improved accessibility. These changes enhance readability and ensure better compliance with modern web standards.
The TestMsgWriter_writeMsg_SMime function was removed as it is no longer relevant or needed. It tested deprecated S/MIME functionality which is no longer maintained, streamlining the test suite and improving maintainability.
Introduces tests to verify S/MIME signing behavior across scenarios such as multipart messages, invalid keys, and a broken rand.Reader. Ensures proper error handling and compatibility with complex message structures like multipart/alternative, multipart/mixed, and multipart/related.
This test was removed as it is redundant or no longer necessary. The cleanup reduces code bloat and improves maintainability of the test suite.
Previously, the code did not properly handle cases where the Certificate field was nil, leading to potential runtime errors. This update adds a nil check to prevent these issues and ensure robust validation.
Replaced redundant `nil` check with a concise length check for `keyPairTLS.Certificate`. This ensures cleaner and more readable code while maintaining the same functionality.
Refactor and extend SMIME test cases for better readability and coverage, including new edge cases and type validations. Introduce comprehensive tests for `getLeafCertificate` function to ensure proper handling of normal, nil, empty chain, and malformed certificate scenarios.
|
The golangci-lint errors are false-positives and can be ignored for the time being. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR extends the S/MIME signing support introduced via #298 (Commit: 804fe25)
This PR changes the following: