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

Question about validation logic #549

Closed
mbrandonw opened this issue Aug 20, 2018 · 3 comments
Closed

Question about validation logic #549

mbrandonw opened this issue Aug 20, 2018 · 3 comments

Comments

@mbrandonw
Copy link

mbrandonw commented Aug 20, 2018

Looking at the source I found this:

var trustedCount = 0
for serverCert in serverCerts {
for cert in certs {
if cert == serverCert {
trustedCount += 1
break
}
}
}
if trustedCount == serverCerts.count {
return true
}

and I'm curious why all of the certificates from the server are required to match one of the certificates provided. I was under the impression that it is sufficient for at least one of the certificates to match.

Is this logic correct?

Thanks!

@mbrandonw mbrandonw changed the title Questiona about validation logic Question about validation logic Aug 20, 2018
@eliburke
Copy link

eliburke commented Aug 20, 2018

@mbrandonw The original behavior was to require the entire chain to be trusted. Like you, I am under the impression that only one cert needs to match. #412 added a boolean to control the behavior. If you set validateEntireChain=false, it will short circuit out of that loop.
If you check the docs for SecTrustEvaluate you'll see that both .proceed and .unspecified are considered success states.

@mbrandonw
Copy link
Author

oh wow, that's it! I saw that option but didn't look further into it. Thanks for the help!

@eliburke
Copy link

I should add-- I left the existing behavior as default in the PR so that current Starscream users wouldn't suddenly have a change in their security model. I do think it should be the default behavior in a future major release such as 4.0 hint hint @daltoniam 😄

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