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

Factor base64 conversions out of authentication procedures #6654

Closed
wants to merge 2 commits into from

Conversation

@monnerat
Copy link
Contributor

@monnerat monnerat commented Feb 24, 2021

Input challenges and returned messages are now in binary.
Conversions from/to base64 are performed by callers (currently curl_sasl.c
and http_ntlm.c).

Binary data control structure authbuffer is introduced with associated
handling primitives. This supports any kind of memory storage, whatever
is the allocation scheme.

This is a first step towards the implementation of binary sasl that will allow full authentication features in openldap.

@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot commented Feb 24, 2021

Congratulations 🎉. DeepCode analyzed your code in 2.831 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@monnerat monnerat force-pushed the monnerat:auth branch from b0e0de7 to 1a8a1fc Feb 24, 2021
lib/vauth/vauth.h Outdated Show resolved Hide resolved
@monnerat monnerat force-pushed the monnerat:auth branch 2 times, most recently from 18f4d1a to 8d53645 Feb 24, 2021
@bagder
Copy link
Member

@bagder bagder commented Feb 25, 2021

Suggestions:

  1. Put the Curl_auth_buffer* functions into its own new file
  2. Maybe call them Curl_authbuffer* - without underscore in the middle - to be consistent with the struct name? Maybe even call it something else since strictly speaking it doesn't have to be about auth at all?
  3. Use accessors to get the pointer and length of data to match with how dynbuf works: Curl_authbuffer_ptr() and Curl_authbuffer_len() - that also don't expose the struct fields.
  4. s/Curl_auth_buffer_alloc/Curl_authbuffer_memdup/ - to better signal that it dupes the memory
  5. A unit test for the Curl_authbuffer* functions
  6. Consider a debug-build-only field in the struct like the dynbuf struct has, that we can do a DEBUGASSERT on to quickly detect wrong-doings like trying to use a non-initialized struct.

To ponder:

In the dynbuf systems we enforce a maximum buffer size at all times to avoid the risk that ridiculously sized values can get set, that then will lead to badness in places (like integer overflows). An extra internal layer of defense. Would it be sensible to make the *set() function refuse to set strings larger than CURL_MAX_INPUT_LENGTH for this sake?

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Feb 25, 2021

I restricted this to auth to avoid name space pollution, but obviously you want to make it available elsewhere by turning it into a "global" support feature and I think you're right :-) Now, how do we call it? Hard to find a representative name. curlbuffer is not meaningful, curlstorage? curlbinbuf? curlfixbuf? curlarea? curldeallocator? maybe you think of something better

Would it be sensible to make the *set() function refuse to set strings larger than CURL_MAX_INPUT_LENGTH for this sake?

Much less than for dynbuf: Unless using Curl_auth_buffer_alloc(), there's no allocation. This kind of check mainly preserves setting a crazy length value, which is used only by callers and Curl_auth_buffer_alloc(). But we can do it anyway (the drawback is we have to test a return status after each call), or at least only as a DEBUGASSERT().

@bagder
Copy link
Member

@bagder bagder commented Feb 25, 2021

obviously you want to make it available elsewhere by turning it into a "global" support feature and I think you're right :-)

Yeah, at least I want to have the ability to do so, and if/when that time comes it will be odd to have this name!

I'm not sure the exact name is terribly important as we'll learn it and associate it with the use we assign to it. It primarily keeps a reference to a (generic) buffer and size. Maybe just call it struct bufref and Curl_bufref_*() ? (As you might know, I'm also personally rather fond of keeping the names short)

@monnerat monnerat force-pushed the monnerat:auth branch 2 times, most recently from a2cf45d to d8d4045 Feb 26, 2021
@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Feb 27, 2021

Now split in 2 commits.
@bagder : All your remarks taken into account. Should be ready now.

Copy link
Member

@bagder bagder left a comment

I'm a big fan!

lib/curl_sasl.c Show resolved Hide resolved
lib/http_ntlm.c Outdated Show resolved Hide resolved
lib/http_ntlm.c Outdated Show resolved Hide resolved
lib/http_ntlm.c Outdated Show resolved Hide resolved
lib/vauth/ntlm.c Outdated Show resolved Hide resolved
lib/vauth/ntlm_sspi.c Outdated Show resolved Hide resolved
@monnerat monnerat force-pushed the monnerat:auth branch from d8d4045 to 6b5d5c1 Mar 11, 2021
@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Mar 11, 2021

@bagder : thanks for your review. Fixups applied according to your remarks.

@monnerat monnerat force-pushed the monnerat:auth branch 2 times, most recently from 4342db5 to 405e73f Mar 17, 2021
@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Mar 22, 2021

@bagder : do you intend to apply this? are you waiting for the next feature window?
The problem is: I have some related work ready but not applicable before this PR:

  • binary sasl
  • a fix for the ntlm logic
  • completely reworked openldap (state machine, starttls, sasl and immediate output + a hack/fix for #6676)

Do I wait for this PR to be pulled or do I include the above in this PR ?

@jay
Copy link
Member

@jay jay commented Mar 22, 2021

Why do you need bufref, can't you use dynbuf? I know you said earlier realloc isn't needed but dynbuf would work well anyway wouldn't it? If separate handling is in fact needed it should have documentation. See docs/DYNBUF.md for example.

Do I wait for this PR to be pulled or do I include the above in this PR ?

Those extra things you listed sound great but not related to this. You could always branch off this PR. I'm for next feature window for this one.

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Mar 22, 2021

Why do you need bufref, can't you use dynbuf? I know you said earlier realloc isn't needed but dynbuf would work well anyway wouldn't it?

No, you can't. bufref's goal is to hold pointers to (preallocated) data and their associated destructor, if some. This allows using static data or data allocated with possibly something else than malloc() to be handled without being copied. In addition, the destructor is set at the same time as the pointer, allowing an easy release with Curl_bufref_free() without checking the origin of the pointer and whatever the associated destructor is. The length parameter is not used by the methods: it's only here to support binary data.

If separate handling is in fact needed it should have documentation.

As it was originally a tiny sasl-private API, I did not document it. If it should be used more widely, so yes, I could write such a document.

@monnerat monnerat force-pushed the monnerat:auth branch from 405e73f to 8aaf964 Mar 23, 2021
@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Mar 23, 2021

docs/BUFREF.md added

@monnerat monnerat force-pushed the monnerat:auth branch 3 times, most recently from b5a2dca to 34ea28f Mar 23, 2021
@monnerat monnerat force-pushed the monnerat:auth branch 3 times, most recently from d475196 to d893f3c Apr 9, 2021
monnerat added 2 commits Mar 23, 2021
A struct bufref holds a buffer pointer, a data size and a destructor.
When freed or its contents are changed, the previous buffer is implicitly
released by the associated destructor. The data size, although not used
internally, allows binary data support.

A unit test checks its handling methods.
Input challenges and returned messages are now in binary.
Conversions from/to base64 are performed by callers (currently curl_sasl.c
and http_ntlm.c).
@monnerat monnerat force-pushed the monnerat:auth branch from d893f3c to bfde423 Apr 19, 2021
@bagder
Copy link
Member

@bagder bagder commented Apr 22, 2021

Thanks!

@bagder bagder closed this in 34cf403 Apr 22, 2021
bagder added a commit that referenced this pull request Apr 22, 2021
Input challenges and returned messages are now in binary.
Conversions from/to base64 are performed by callers (currently curl_sasl.c
and http_ntlm.c).

Closes #6654
@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Apr 22, 2021

Thanks for pulling.

@monnerat monnerat deleted the monnerat:auth branch Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants