Added support for RFC7616 - HTTP Digest access authentication #1934

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@FlorinPetriuc

Added support for RFC7616 in curl client - HTTP Digest access authentication

Signed-off-by: Florin petriuc.florin@gmail.com

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 30, 2017

Member

Awesome, thanks a lot! This is the CI error:
sha256.c:180:2: error: no newline at end of file [-Werror,-Wnewline-eof]

Member

bagder commented Sep 30, 2017

Awesome, thanks a lot! This is the CI error:
sha256.c:180:2: error: no newline at end of file [-Werror,-Wnewline-eof]

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Oct 1, 2017

Member

Did you write that sha256 implementation, and if not we need to know where it came from to resolve any potential licensing issues

Member

jay commented Oct 1, 2017

Did you write that sha256 implementation, and if not we need to know where it came from to resolve any potential licensing issues

@FlorinPetriuc

This comment has been minimized.

Show comment
Hide comment
@FlorinPetriuc

FlorinPetriuc Oct 1, 2017

I am working on the CI errors now. The sha256 implementation is from here: https://github.com/B-Con/crypto-algorithms/blob/master/sha256.c, but I had to change the function names and protoypes to match the ones from openssl.

FlorinPetriuc commented Oct 1, 2017

I am working on the CI errors now. The sha256 implementation is from here: https://github.com/B-Con/crypto-algorithms/blob/master/sha256.c, but I had to change the function names and protoypes to match the ones from openssl.

@FlorinPetriuc

This comment has been minimized.

Show comment
Hide comment
@FlorinPetriuc

FlorinPetriuc Oct 1, 2017

CI seems to give this error which I am not sure how to go around:

error: ./../docs/libcurl/libcurl-tutorial.3:628: use \fI before curl_slist_free_all(3)
error: ./../docs/libcurl/libcurl-tutorial.3:1216: refering to non-existing man page CURLOPT_HTTPHEADERS.3

It is failing test 1140.

Any suggestions?

CI seems to give this error which I am not sure how to go around:

error: ./../docs/libcurl/libcurl-tutorial.3:628: use \fI before curl_slist_free_all(3)
error: ./../docs/libcurl/libcurl-tutorial.3:1216: refering to non-existing man page CURLOPT_HTTPHEADERS.3

It is failing test 1140.

Any suggestions?

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Oct 2, 2017

Member

The sha256 implementation is from here: https://github.com/B-Con/crypto-algorithms/blob/master/sha256.c, but I had to change the function names and protoypes to match the ones from openssl.

"This code is released into the public domain free of any restrictions. The author requests acknowledgement if the code is used, but does not require it. This code is provided free of any liability and without any quality claims by the author."

Please mention that in the source like "Based on SHA-256 implementation by Brad Conte (brad AT bradconte.com)" and url. Also it is only little endian but we need big endian support as well.

Also does this need to be taken into consideration:

"Note that these are not cryptographically secure implementations. They have no resistence to side-channel attacks and should not be used in contexts that need cryptographically secure implementations."

It is failing test 1140

Should be fixed in 753a5da, can you rebase on master and let the CI run again?

Member

jay commented Oct 2, 2017

The sha256 implementation is from here: https://github.com/B-Con/crypto-algorithms/blob/master/sha256.c, but I had to change the function names and protoypes to match the ones from openssl.

"This code is released into the public domain free of any restrictions. The author requests acknowledgement if the code is used, but does not require it. This code is provided free of any liability and without any quality claims by the author."

Please mention that in the source like "Based on SHA-256 implementation by Brad Conte (brad AT bradconte.com)" and url. Also it is only little endian but we need big endian support as well.

Also does this need to be taken into consideration:

"Note that these are not cryptographically secure implementations. They have no resistence to side-channel attacks and should not be used in contexts that need cryptographically secure implementations."

It is failing test 1140

Should be fixed in 753a5da, can you rebase on master and let the CI run again?

@FlorinPetriuc

This comment has been minimized.

Show comment
Hide comment
@FlorinPetriuc

FlorinPetriuc Oct 2, 2017

I did the rebase. Hopefully the CI will complete successfully. I have changed the sha implementation and adapted the one from mbedtls (https://github.com/ARMmbed/mbedtls) which should be cryptographically secure. It should support both big and little endian.

I did the rebase. Hopefully the CI will complete successfully. I have changed the sha implementation and adapted the one from mbedtls (https://github.com/ARMmbed/mbedtls) which should be cryptographically secure. It should support both big and little endian.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Oct 2, 2017

Member

Ok. @bagder sha256 is now using an implementation released under apache 2.0 license, I don't know whether that's acceptable or not, it appears to have more requirements than MIT

Member

jay commented Oct 2, 2017

Ok. @bagder sha256 is now using an implementation released under apache 2.0 license, I don't know whether that's acceptable or not, it appears to have more requirements than MIT

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 2, 2017

Member

The Apache 2 license is inconvenient for us because it is considered incompatible with GPLv2, so a curl using a file with that code can't be linked with GPLv2 projects!

Member

bagder commented Oct 2, 2017

The Apache 2 license is inconvenient for us because it is considered incompatible with GPLv2, so a curl using a file with that code can't be linked with GPLv2 projects!

@FlorinPetriuc

This comment has been minimized.

Show comment
Hide comment
@FlorinPetriuc

FlorinPetriuc Oct 2, 2017

What about the openssl implementation? I can look into porting that if it's ok.

What about the openssl implementation? I can look into porting that if it's ok.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 2, 2017

Member

The ideal solution would be to use the sha256 implementation that's already in the TLS library that's most likely used as well, which is how the keypinning code does it. OpenSSL is also Apache 2 these days and before that they had another GPL incompatible license so that's not ideal either. The best licenses for us would be MIT or BSD.

Member

bagder commented Oct 2, 2017

The ideal solution would be to use the sha256 implementation that's already in the TLS library that's most likely used as well, which is how the keypinning code does it. OpenSSL is also Apache 2 these days and before that they had another GPL incompatible license so that's not ideal either. The best licenses for us would be MIT or BSD.

@FlorinPetriuc

This comment has been minimized.

Show comment
Hide comment
@FlorinPetriuc

FlorinPetriuc Oct 2, 2017

Alright. I will try to do it this week.

Alright. I will try to do it this week.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 2, 2017

Member
Member

bagder commented Oct 2, 2017

@FlorinPetriuc

This comment has been minimized.

Show comment
Hide comment
@FlorinPetriuc

FlorinPetriuc Oct 3, 2017

I added a public domain SHA256 implementation today. It's from libtomcrypt and it was released to public domain. It was easier to port that one.

I added a public domain SHA256 implementation today. It's from libtomcrypt and it was released to public domain. It was easier to port that one.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 5, 2017

Member

Awesome, can you look into creating a test case or two as well?

I suppose this code path is triggered automatically based on the header contents, so there's really nothing particular to document? Maybe CURLOPT_HTTPAUTH.3 and CURLOPT_PROXYAUTH.3 at least needs a mention somewhere that we support the RFC7616 style starting with 7.57.0 ?

Member

bagder commented Oct 5, 2017

Awesome, can you look into creating a test case or two as well?

I suppose this code path is triggered automatically based on the header contents, so there's really nothing particular to document? Maybe CURLOPT_HTTPAUTH.3 and CURLOPT_PROXYAUTH.3 at least needs a mention somewhere that we support the RFC7616 style starting with 7.57.0 ?

@FlorinPetriuc

This comment has been minimized.

Show comment
Hide comment
@FlorinPetriuc

FlorinPetriuc Oct 5, 2017

Alright. Yes.. it is automatic. I'll add some test cases this week and update those 2 docs.

Alright. Yes.. it is automatic. I'll add some test cases this week and update those 2 docs.

Florin added some commits Sep 30, 2017

Florin
Added client support for RFC7616 - HTTP Digest access authentication
Signed-off-by: Florin <petriuc.florin@gmail.com>
Florin
Added test cases for RFC7616
Updated docs to include support for RFC7616

Signed-off-by: Florin <petriuc.florin@gmail.com>
@FlorinPetriuc

This comment has been minimized.

Show comment
Hide comment
@FlorinPetriuc

FlorinPetriuc Oct 7, 2017

Added some test cases that I thought were relevant.

Added some test cases that I thought were relevant.

@c4xp

This comment has been minimized.

Show comment
Hide comment
@c4xp

c4xp Oct 12, 2017

Finally, good work Florin :) 👍
P.S.

c4xp commented Oct 12, 2017

Finally, good work Florin :) 👍
P.S.

@bagder bagder closed this in f20cbac Oct 28, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 28, 2017

Member

Thanks a lot @FlorinPetriuc for your hard work on this. Merged now!

Member

bagder commented Oct 28, 2017

Thanks a lot @FlorinPetriuc for your hard work on this. Merged now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment