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

about function loadSystemRoots #100

Open
lysShub opened this issue Oct 1, 2024 · 13 comments
Open

about function loadSystemRoots #100

lysShub opened this issue Oct 1, 2024 · 13 comments

Comments

@lysShub
Copy link

lysShub commented Oct 1, 2024

  1. if !strings.Contains(string(out), "command completed successfully") { not work on other language system;maybe if cmd.ProcessState.ExitCode() != 0 {
@lysShub
Copy link
Author

lysShub commented Oct 1, 2024

  1. certutil -syncWithWU cost too many time, recommend use certutil -verify directly

@ayoubfaouzi
Copy link
Member

@lysShub thanks for your feedback.

I agree we should check exit code regarding point number 1.
For point 2: do you know if verify does grab the lastest certs ?

@lysShub
Copy link
Author

lysShub commented Oct 7, 2024

@lysShub thanks for your feedback.

I agree we should check exit code regarding point number 1. For point 2: do you know if verify does grab the lastest certs ?

my test result is, when a required cert missing:
    if exec 'certutil verify xxx.crt' with administrator access, the system will automatically download the missing cert
    otherwise, will verify fail

@lysShub
Copy link
Author

lysShub commented Oct 16, 2024

not need to manually create CertPool, if VerifyOptions.Roots is nil and golang will call system API to verify: https://github.com/golang/go/blob/go1.22.0/src/crypto/x509/verify.go#L770

@LordNoteworthy

@ayoubfaouzi
Copy link
Member

Thanks a lot @lysShub, I should be able to fix this by the end of the week

@ayoubfaouzi
Copy link
Member

Hey @lysShub

-verify requires us to first download the certs, so do we have to first run the sync then in subsequent commands do the verification ?

Can you post a quick benchmark how this is faster ?

My tests shows that when sync runs the first time, its slow, then subsequent executions are fast.

@lysShub
Copy link
Author

lysShub commented Oct 30, 2024

@LordNoteworthy I mean is that no need the certPool, but directly:

	// Let's load the system root certs.
	if !pe.opts.DisableCertValidation {
		certValid = pkcs.Verify() == nil
	}

@ayoubfaouzi
Copy link
Member

I see, does this work prior to go v1.22 ?

@lysShub
Copy link
Author

lysShub commented Nov 18, 2024

yes, it depend on Verify,not need to distinguish Windows or Linux

@ayoubfaouzi
Copy link
Member

ayoubfaouzi commented Nov 18, 2024

image

Verify is just a wrapper over VerityWithChain. It does not seem to do that?

@ayoubfaouzi
Copy link
Member

@lysShub
Copy link
Author

lysShub commented Nov 18, 2024

to be continue, finally call windows system function, such as syscall.CertGetCertificateChain

@ayoubfaouzi
Copy link
Member

	// Let's load the system root certs.
	if !pe.opts.DisableCertValidation {
		certValid = pkcs.Verify() == nil
	}

The unit tests fails when I changed it as above. Can you please submit a PR ?

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

No branches or pull requests

2 participants