Validate subjAltName for uuid and similar URNs #5964
Conversation
… Studio 2022 default).
…ifiers (RFC 9039) and UUIDs (RFC 4122)
|
Can one of the admins verify this patch? |
|
Hi @PaulMartinsen, Thanks for the bug report and PR. We do require a signed contributor agreement on file for us to accept external contributions. If you are interested, please reach out to us at support [at] wolfssl [dot] com for a copy of our contributor agreement. |
| @@ -42,46 +42,46 @@ | |||
| <Import Project="$(VCTargetsPath)\Microsoft.Cpp.Default.props" /> | |||
| <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'" Label="Configuration"> | |||
| <ConfigurationType>Application</ConfigurationType> | |||
| <PlatformToolset>v110</PlatformToolset> | |||
| <PlatformToolset>v143</PlatformToolset> | |||
There was a problem hiding this comment.
We cannot accept the visual studio project changes. Those must be reverted.
There was a problem hiding this comment.
I understand the need to continue to have the VS2012 files for existing customers. Any objection to including additional project files that target more modern, such as VS2022? Perhaps a naming convention of simply adding _VS2022 to the project file name?
There was a problem hiding this comment.
It is great you're continuing to support older versions of the VS toolchain such as VS2012 and VS2005. Though it was a very big red flag to me to see reliance on a decade old complier & standard library as the default selection as the primary Windows build path in a security library.
As the VS2012 toolchain is no longer available outside specific MSDN subscriptions, I suggest you create additional 2012 project files for legacy users and update to a more modern toolchain that will make it easier for the majority of new Windows users to work with your library. This has the additional benefit of clearly surfacing the v110 build chain support.
I have updated the PR as suggested, targeting the v143 toolchain. This can be used from the most recent versions of Visual Studio, which automatically prompts the user to download the toolchain if it isn't installed.
Note, sslSniffTest was not changed as it targets the v141 toolchain (VS2017), and is still readily available.
| * chars are "//" to match the pattern "://" */ | ||
| if (i >= strLen - 2 || (input[idx + i + 1] != '/' || | ||
| input[idx + i + 2] != '/')) { | ||
| /* test if scheme is missing or heir-part is empty */ |
There was a problem hiding this comment.
We need a test case for the new feature.
There was a problem hiding this comment.
Yes. And, as I mentioned in the PR, I'm happy to write it if someone will generate a suitable certificate. Unfortunately, I wasn't able to follow the generation of test certificates in the library test suite to do it myself.
Here's an example of how I generate the signing request through OpenSSL:
set DeviceId=9259c005-fa68-4857-b0e7-683bc4236f26
%OPENSSL% req -new -newkey rsa:4096 -keyout Output\Device-PrivateKey-%DeviceId%.pem ^
-out Output\Device-CertificateRequest-%DeviceId%.pem ^
-passout pass:secret ^
-subj "/C=NZ/ST=Waikato/L=Hamilton/O=Blue Leaf Software Ltd/CN=Lumos/serialNumber=%DeviceId%" ^
-addext "subjectAltName = URI:uuid:%DeviceId%" ^
-addext "extendedKeyUsage = serverAuth, clientAuth"
Please let me know when you have a suitable certificate ready.
… (Visual Studio 2022 default)." This reverts commit 5a5ad49.
|
Hello @PaulMartinsen , Thank you for your contribution. I have had a separate customer request for absolute URN support, and I need it in our upcoming release, so I've integrated your fix into my PR here: #6181 Thanks, |
|
Hi @kareem-wolfssl ; I don't see any issues. Will be good to get this incorporated. |
|
Hello @PaulMartinsen - It looks like this was a good and approved update, but there's currently a conflict with Would you like to resolve the commit conflict? |
|
Hi @gojimmypi , it sounds like @kareem-wolfssl added these changes to #PR6181 , which has already been merged. So I think this one might be orphaned? Is that right @kareem-wolfssl ? |
|
@PaulMartinsen getting back to some of the lingering pull requests. You're right that the other pull request from Kareem addressed the same issue that this one did. Closing this out in favor of the already merged one. Thanks again for opening this and feel free to reach out with any issues or code changes in the future. |
Description
An attempt to improve validation of subjAltName to allow UUID (RFC 4122) and URNs for device identifiers (RFC 9039) in subjAltName field, but still ensuring the URN is not relative. For #5963 .
Testing
Tested
It would be good to add another test to the testsuite with a certificate that uses, for example, a UUID in the subjAltName. Could someone generate a suitable certificate that fits the bill please? Then I could add the test. I don't want to put our own certificates in the repo!