8000 schannel: Fix certificate error reporting and ignore some trust errors by default by lrm29 · Pull Request #6941 · libgit2/libgit2 · GitHub
[go: up one dir, main page]

Skip to content

schannel: Fix certificate error reporting and ignore some trust errors by default #6941

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

lrm29
Copy link
Contributor
@lrm29 lrm29 commented Nov 17, 2024

This is a partial fix for #6724.

  1. dwErrorStatus is a bitmask and so I've attempted to concatenate the error strings.
  2. libgit2 now uses the same defaults as Git for Windows, ignoring CERT_TRUST_REVOCATION_STATUS_UNKNOWN and CERT_TRUST_IS_OFFLINE_REVOCATION.

I would add support for querying "http.schannelcheckrevoke", but am not sure how to get a repository config snapshot in this function. Or could just get the global config. I didn't see any stream implementations that query the config.

@lrm29
Copy link
Contributor Author
lrm29 commented Dec 11, 2024

Test fails (not when I run it locally though), meaning the CI environment must be different. Might be a tricky one to test...

@ethomson
Copy link
Member

I think that you're testing the literal error messages that come out of schannel and the Linux and Mac implementations are slightly different messages.

git_clone(
&g_repo, "https://untrusted-root.badssl.com/fake.git",
"./fake", NULL));
cl_assert_equal_i(git_error_last()->klass, GIT_ERROR_SSL);
Copy link
Contributor Author
@lrm29 lrm29 Dec 11, 2024

Choose a reason for hiding this comment

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

With WinHTTP this error code would be GIT_ERROR_HTTP, and as pointed out the message strings are very different with all the other implementations (I should do this stuff in the morning...).

This test is a bit pointless I suppose as it's just testing error message concatenation. What I really want to test is this logic that I added, but I'm not sure how:

if ((st->cert_chain->TrustStatus.dwErrorStatus &
	     ~(DWORD)(CERT_TRUST_REVOCATION_STATUS_UNKNOWN |
	              CERT_TRUST_IS_OFFLINE_REVOCATION)) != CERT_TRUST_NO_ERROR)

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
0