search

simplesamlphp / simplesamlphp-module-negotiate

Negotiate module
GNU Lesser General Public License v2.1
4 stars 5 forks source link

Implement certificate-based channel binding #12

Closed tvdijen closed 4 months ago

tvdijen commented 5 months ago

Depends on https://github.com/php/pecl-authentication-krb5/pull/3 to be merged and released.

@thijskh I don't expect you to understand too much of this PR because I know you don't use this module, but would you mind taking a look?

The 'deal' here is, we receive a ticket with a certain 'context'.. Context being the certificate fingerprint using while retrieving the ticket. We compare the fingerprint received with the configured value to prevent MitM attacks.

Key is that the new option is optional; we can either leave it away to not perform this check, or we pass an array of certificate hashes and pass the test if any of the hashes are good. The complex part here that I'm worried about is the array of hashes being processed..

If either hash matches, authentication succeeds. If none match, we fail.

codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 2.19780% with 89 lines in your changes are missing coverage. Please review.

Project coverage is 26.29%. Comparing base (ba99560) to head (2eea81f). Report is 2 commits behind head on release-2.2.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## release-2.2 #12 +/- ## ================================================= - Coverage 32.51% 26.29% -6.22% - Complexity 55 68 +13 ================================================= Files 2 2 Lines 203 251 +48 ================================================= Hits 66 66 - Misses 137 185 +48 ```
tvdijen commented 5 months ago

@ArnoutvdKnaap Please review

ArnoutvdKnaap commented 5 months ago

Looking at the code this would work, however if there is a hashing algorithm in the certificate and it is not MD5 or SHA1 the hashing algorithm is taken from the certificate (if in there otherwise sha-256). It might be usefull to put the ordering of the hashes that will be validated into the md file or maybe a link to the RFC?

tvdijen commented 5 months ago

I think today's certificates always have a SHA-256 fingerprint, unless I'm misunderstanding you?

ArnoutvdKnaap commented 5 months ago

The certificate may specify a certificate hashing algorithm, If it does and it is not MD5 or SHA1 (because those will be overwritten to SHA-256) that algorithm should be used to hash the certificate. Thus if the certificate specifies that SHA-512 should be used that is the hash that needs to be added.

tvdijen commented 5 months ago

That's something we should add to the docs!

ArnoutvdKnaap commented 5 months ago

1 more thing, if no setting is provided. SHA-256 is used