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

Added support for RFC7616 - HTTP Digest access authentication #1934

Closed
wants to merge 2 commits into from

Conversation

FlorinPetriuc
Copy link

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

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

@bagder
Copy link
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
Copy link
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
Copy link
Author

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
Copy link
Author

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
Copy link
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
Copy link
Author

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
Copy link
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
Copy link
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
Copy link
Author

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

@bagder
Copy link
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
Copy link
Author

Alright. I will try to do it this week.

@bagder
Copy link
Member

bagder commented Oct 2, 2017

Here's a BSD licensed sha256.

@FlorinPetriuc
Copy link
Author

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
Copy link
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
Copy link
Author

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

Signed-off-by: Florin <petriuc.florin@gmail.com>
Updated docs to include support for RFC7616

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

Added some test cases that I thought were relevant.

@c4xp
Copy link

c4xp commented Oct 12, 2017

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

@bagder bagder closed this in f20cbac Oct 28, 2017
@bagder
Copy link
Member

bagder commented Oct 28, 2017

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

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

4 participants