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

[Windows] Fix access denied issue in OVS cert import #6529

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

wenyingd
Copy link
Contributor

@wenyingd wenyingd commented Jul 17, 2024

An "Access is denied" error is possibly returned when importing certificate into the trusted publishers store at the first time on a fresh Windows 2022 Node.

To resolve the issue, this change uses the "Add" method provided by certificate stre as an alternative when importing to trusted publishers.

Fix: #6530

@wenyingd wenyingd requested a review from XinShuYang July 17, 2024 05:48
@XinShuYang
Copy link
Contributor

/test-windows-all

# performed on a fresh Windows 2022 Node.
$CertStore = Get-Item cert:\LocalMachine\TrustedPublisher
$CertStore.Open([System.Security.Cryptography.X509Certificates.OpenFlags]"ReadWrite")
$CertStore.Add($(Get-Item $CertificateFile).FullName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Import the certificate from the file into an X509Certificate2 object and then adds that object to the store !

Suggested change
$CertStore.Add($(Get-Item $CertificateFile).FullName)
# Load the certificate from the file into an X509Certificate2 object
$certificate = New-Object System.Security.Cryptography.X509Certificates.X509Certificate2
$certificate.Import($certificateFilePath)
# Add the certificate to the store
$certStore.Add($certificate)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rajnkamr , I have verified that my solution does work. I don't think it is necessary to explicitly create a new object for the certificate, instead, when we use a file path to add into the cert store, Windows OS would load the cert automatically. My current solution (using a file path as parameter) is supported by Windows.

@wenyingd
Copy link
Contributor Author

/test-windows-all

@wenyingd
Copy link
Contributor Author

/test-all

@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label Jul 17, 2024
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM, a nit about the comment.

Comment on lines 180 to 181
# (issue #6530) is possibly hit when the import-certificate to trusted publisher store is firstly
# performed on a fresh Windows 2022 Node.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# (issue #6530) is possibly hit when the import-certificate to trusted publisher store is firstly
# performed on a fresh Windows 2022 Node.
# may occur when `Import-Certificate` is used to import a certificate to the trusted publisher
# store for the first time on a fresh Windows 2022 Node. See issue #6530.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

@tnqn
Copy link
Member

tnqn commented Jul 17, 2024

Does this need to be backported?

@wenyingd
Copy link
Contributor Author

Does this need to be backported?

I don't think so, the changes to import cert into both trusted store and root store for all versions (including both signed and unsigned cert) is introduced in this version (v2.1), and the "Access is denied" is seen on Windows Server 2022 which is firstly supported in v2.1 too. So it shall be OK to keep the change in this version.

XinShuYang
XinShuYang previously approved these changes Jul 18, 2024
An "Access is denied" error is possibly returned when importing certificate into
the trusted publishers store at the first time on a fresh Windows 2022 Node.

To resolve the issue, this change uses the "Add" method provided by certificate
stre as an alternative when importing to trusted publishers.

Signed-off-by: Wenying Dong <wenyingd@vmware.com>
@wenyingd
Copy link
Contributor Author

/test-windows-all
/test-all

@tnqn
Copy link
Member

tnqn commented Jul 18, 2024

I don't think so, the changes to import cert into both trusted store and root store for all versions (including both signed and unsigned cert) is introduced in this version (v2.1), and the "Access is denied" is seen on Windows Server 2022 which is firstly supported in v2.1 too. So it shall be OK to keep the change in this version.

Got it, then I will remove action/release-note too as this should be part of another change.

@tnqn tnqn removed the action/release-note Indicates a PR that should be included in release notes. label Jul 18, 2024
@wenyingd
Copy link
Contributor Author

All windows tests are passed in our CI testbed (I tried 3 rounds of test, and all passed), and the change is also verified on a setup with fresh Windows 2022 Nodes. Can we move it forward? @tnqn

@tnqn tnqn merged commit 6e4ff87 into antrea-io:main Jul 18, 2024
58 of 61 checks passed
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.

Antrea Windows initContainer fails to load by cert import error
4 participants