You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@serf.apache.org by Branko Čibej <br...@apache.org> on 2025/07/08 15:26:01 UTC
Re: Thinking about MFA infrastructure
On 26. 6. 25 08:16, Branko Čibej wrote:
> On 24. 6. 25 07:52, Branko Čibej wrote:
>> On 14. 6. 25 21:23, Branko Čibej wrote:
>>> Hi all,
>>>
>>> I've been digging into Serf's support for various authentication
>>> schemes and I notices something that looks like a bit of a limitation.
>>>
>>> Unless I'm much mistaken, there's space for only one authentication
>>> baton in Serf's context. It would seem that this is rather a blocker
>>> for implementing multi-factor authentication flows, for example,
>>> Basic + OTP, where the server would first require basic credentials
>>> and then, if those were correct, go on to issue an OTP challenge.
>>>
>>> It seems to me that a simple solution for that would be to store an
>>> authn baton per scheme, but I know on the close order of nothing
>>> about the possible side effects.
>>>
>>> Yeah, I'm starting small, I have no wish to implement OAuth2 flow
>>> any time soon. Still, a bit of insight from the knowledgeable would
>>> be welcome.
>>
>>
>> With r1926674, I think the user-defined-authn branch is ready for
>> review. Here's a short summary of the changes on the branch:
>>
>> * The (private) struct serf__authn_scheme_t, defined in auth/auth.h,
>> grew a number of new members to support user-defined schemes, among
>> them five new user-visible callback functions. These, along with
>> their interactions, are described in serf.h. The built-in
>> authentication schemes don't need or use these new members.
>>
>> * The (private) table serf_authn_schemes, defined in auth/auth.c, has
>> been extended to have room for as many schemes as there are bits in
>> serf__authn_scheme_t::type. Locking was added to serialize access,
>> because new schemes can be registered at runtime.
>>
>> * Private callbacks were added to implement the user-visible
>> authentication flow.
>>
>> * Two new public APIs take care of registering and unregistering
>> authentication schemes. The latter is tricky because we don't track
>> references to the schemes in contexts and connections, only in the
>> serf_authn_schemes table; this issue is documented.
>>
>> * A number of tests were added for all this new stuff.
>>
>>
>> Please take a look if you have time.
>>
>> svn diff --internal-diff \
>> https://svn.apache.org/repos/asf/serf/trunk \
>> https://svn.apache.org/repos/asf/serf/branches/user-defined-authn \
>> | colordiff --color=always \
>> | less -R
>>
>
> There are a couple more changes on that branch now:
>
> * The parameters in the authentication challenge (headers
> WWW-Authenticate and Proxy-Authenticate) are now parsed into a
> dictionary, using the rules described in RFC 9110.
>
> * The Authentication-Info and Proxy-Authentication-Info response
> headers are now handled and their values parsed as above.
>
> These two changes should make life much easier for authn scheme
> implementers. I decided to add this once I saw how many times we have
> copy-pasted not-quite-compliant parsers throughout the auth code...
>
> (Note that these changes do not stop implementers from doing whatever
> wrong-headed, non-compliant horror they can think of, as all the raw
> information and headers are still available in the callbacks.)
Any takers? It's been a couple of weeks now. I'd sure like to get
another pair of eyes on these changes before merging them to trunk, but
I'd rather not have the branch fall victim to bitrot, either.
-- Brane
Re: Thinking about MFA infrastructure
Posted by Branko Čibej <br...@apache.org>.
On 8. 7. 25 22:22, Dirk-Willem van Gulik wrote:
> On 8 Jul 2025, at 17:26, Branko Čibej<br...@apache.org> wrote:
>> On 26. 6. 25 08:16, Branko Čibej wrote:
>>> On 24. 6. 25 07:52, Branko Čibej wrote:
>>>> On 14. 6. 25 21:23, Branko Čibej wrote:
>>>>> Hi all,
>>>>>
>>>>> I've been digging into Serf's support for various authentication schemes and I notices something that looks like a bit of a limitation.
>>>>>
>>>>> Unless I'm much mistaken, there's space for only one authentication baton in Serf's context. It would seem that this is rather a blocker for implementing multi-factor authentication flows, for example, Basic + OTP, where the server would first require basic credentials and then, if those were correct, go on to issue an OTP challenge.
>>>>>
>>>>> It seems to me that a simple solution for that would be to store an authn baton per scheme, but I know on the close order of nothing about the possible side effects.
>>>>>
>>>>> Yeah, I'm starting small, I have no wish to implement OAuth2 flow any time soon. Still, a bit of insight from the knowledgeable would be welcome.
>>>>
>>>> With r1926674, I think the user-defined-authn branch is ready for review. Here's a short summary of the changes on the branch:
>>>>
>>>> * The (private) struct serf__authn_scheme_t, defined in auth/auth.h,
>>>> grew a number of new members to support user-defined schemes, among
>>>> them five new user-visible callback functions. These, along with
>>>> their interactions, are described in serf.h. The built-in
>>>> authentication schemes don't need or use these new members.
>>>>
>>>> * The (private) table serf_authn_schemes, defined in auth/auth.c, has
>>>> been extended to have room for as many schemes as there are bits in
>>>> serf__authn_scheme_t::type. Locking was added to serialize access,
>>>> because new schemes can be registered at runtime.
>>>>
>>>> * Private callbacks were added to implement the user-visible
>>>> authentication flow.
>>>>
>>>> * Two new public APIs take care of registering and unregistering
>>>> authentication schemes. The latter is tricky because we don't track
>>>> references to the schemes in contexts and connections, only in the
>>>> serf_authn_schemes table; this issue is documented.
>>>>
>>>> * A number of tests were added for all this new stuff.
>>>>
>>>>
>>>> Please take a look if you have time.
>>>>
>>>> svn diff --internal-diff \
>>>> https://svn.apache.org/repos/asf/serf/trunk \
>>>> https://svn.apache.org/repos/asf/serf/branches/user-defined-authn \
>>>> | colordiff --color=always \
>>>> | less -R
>>>>
>>> There are a couple more changes on that branch now:
>>>
>>> * The parameters in the authentication challenge (headers
>>> WWW-Authenticate and Proxy-Authenticate) are now parsed into a
>>> dictionary, using the rules described in RFC 9110.
>>>
>>> * The Authentication-Info and Proxy-Authentication-Info response
>>> headers are now handled and their values parsed as above.
>>>
>>> These two changes should make life much easier for authn scheme implementers. I decided to add this once I saw how many times we have copy-pasted not-quite-compliant parsers throughout the auth code...
>>>
>>> (Note that these changes do not stop implementers from doing whatever wrong-headed, non-compliant horror they can think of, as all the raw information and headers are still available in the callbacks.)
>> Any takers? It's been a couple of weeks now. I'd sure like to get another pair of eyes on these changes before merging them to trunk, but I'd rather not have the branch fall victim to bitrot, either.
> I'll bite - so I've tried it against some auth testing code we privately maintain for legacy hardware keys - and it certainly gets rid of a lot of the boilerplate/header parse code - without loss of functionality. And oddly it passes now an EAGAIN/EINTR edge case that always broke our tests.
>
> Improvement overall.
Thanks for testing! This is encouraging.
-- Brane
Re: Thinking about MFA infrastructure
Posted by Daniel Sahlberg <da...@gmail.com>.
Den tis 8 juli 2025 kl 22:23 skrev Dirk-Willem van Gulik <
dirkx@webweaving.org>:
> On 8 Jul 2025, at 17:26, Branko Čibej <br...@apache.org> wrote:
> > On 26. 6. 25 08:16, Branko Čibej wrote:
> >> On 24. 6. 25 07:52, Branko Čibej wrote:
> >>> On 14. 6. 25 21:23, Branko Čibej wrote:
> >>>> Hi all,
> >>>>
> >>>> I've been digging into Serf's support for various authentication
> schemes and I notices something that looks like a bit of a limitation.
> >>>>
> >>>> Unless I'm much mistaken, there's space for only one authentication
> baton in Serf's context. It would seem that this is rather a blocker for
> implementing multi-factor authentication flows, for example, Basic + OTP,
> where the server would first require basic credentials and then, if those
> were correct, go on to issue an OTP challenge.
> >>>>
> >>>> It seems to me that a simple solution for that would be to store an
> authn baton per scheme, but I know on the close order of nothing about the
> possible side effects.
> >>>>
> >>>> Yeah, I'm starting small, I have no wish to implement OAuth2 flow any
> time soon. Still, a bit of insight from the knowledgeable would be welcome.
> >>>
> >>>
> >>> With r1926674, I think the user-defined-authn branch is ready for
> review. Here's a short summary of the changes on the branch:
> >>>
> >>> * The (private) struct serf__authn_scheme_t, defined in auth/auth.h,
> >>> grew a number of new members to support user-defined schemes, among
> >>> them five new user-visible callback functions. These, along with
> >>> their interactions, are described in serf.h. The built-in
> >>> authentication schemes don't need or use these new members.
> >>>
> >>> * The (private) table serf_authn_schemes, defined in auth/auth.c, has
> >>> been extended to have room for as many schemes as there are bits in
> >>> serf__authn_scheme_t::type. Locking was added to serialize access,
> >>> because new schemes can be registered at runtime.
> >>>
> >>> * Private callbacks were added to implement the user-visible
> >>> authentication flow.
> >>>
> >>> * Two new public APIs take care of registering and unregistering
> >>> authentication schemes. The latter is tricky because we don't track
> >>> references to the schemes in contexts and connections, only in the
> >>> serf_authn_schemes table; this issue is documented.
> >>>
> >>> * A number of tests were added for all this new stuff.
> >>>
> >>>
> >>> Please take a look if you have time.
> >>>
> >>> svn diff --internal-diff \
> >>> https://svn.apache.org/repos/asf/serf/trunk \
> >>> https://svn.apache.org/repos/asf/serf/branches/user-defined-authn \
> >>> | colordiff --color=always \
> >>> | less -R
> >>>
> >>
> >> There are a couple more changes on that branch now:
> >>
> >> * The parameters in the authentication challenge (headers
> >> WWW-Authenticate and Proxy-Authenticate) are now parsed into a
> >> dictionary, using the rules described in RFC 9110.
> >>
> >> * The Authentication-Info and Proxy-Authentication-Info response
> >> headers are now handled and their values parsed as above.
> >>
> >> These two changes should make life much easier for authn scheme
> implementers. I decided to add this once I saw how many times we have
> copy-pasted not-quite-compliant parsers throughout the auth code...
> >>
> >> (Note that these changes do not stop implementers from doing whatever
> wrong-headed, non-compliant horror they can think of, as all the raw
> information and headers are still available in the callbacks.)
> >
> > Any takers? It's been a couple of weeks now. I'd sure like to get
> another pair of eyes on these changes before merging them to trunk, but I'd
> rather not have the branch fall victim to bitrot, either.
>
> I'll bite - so I've tried it against some auth testing code we privately
> maintain for legacy hardware keys - and it certainly gets rid of a lot of
> the boilerplate/header parse code - without loss of functionality. And
> oddly it passes now an EAGAIN/EINTR edge case that always broke our tests.
>
> Improvement overall.
>
> Dw
Thanks for testing! It is on my todo list to review it but it seems there
are always new issues / pr popping up and I've not been able to prioritise
this. My goal is to do it before the end of summer vacations...
/Daniel
Re: Thinking about MFA infrastructure
Posted by Dirk-Willem van Gulik <di...@webweaving.org>.
On 8 Jul 2025, at 17:26, Branko Čibej <br...@apache.org> wrote:
> On 26. 6. 25 08:16, Branko Čibej wrote:
>> On 24. 6. 25 07:52, Branko Čibej wrote:
>>> On 14. 6. 25 21:23, Branko Čibej wrote:
>>>> Hi all,
>>>>
>>>> I've been digging into Serf's support for various authentication schemes and I notices something that looks like a bit of a limitation.
>>>>
>>>> Unless I'm much mistaken, there's space for only one authentication baton in Serf's context. It would seem that this is rather a blocker for implementing multi-factor authentication flows, for example, Basic + OTP, where the server would first require basic credentials and then, if those were correct, go on to issue an OTP challenge.
>>>>
>>>> It seems to me that a simple solution for that would be to store an authn baton per scheme, but I know on the close order of nothing about the possible side effects.
>>>>
>>>> Yeah, I'm starting small, I have no wish to implement OAuth2 flow any time soon. Still, a bit of insight from the knowledgeable would be welcome.
>>>
>>>
>>> With r1926674, I think the user-defined-authn branch is ready for review. Here's a short summary of the changes on the branch:
>>>
>>> * The (private) struct serf__authn_scheme_t, defined in auth/auth.h,
>>> grew a number of new members to support user-defined schemes, among
>>> them five new user-visible callback functions. These, along with
>>> their interactions, are described in serf.h. The built-in
>>> authentication schemes don't need or use these new members.
>>>
>>> * The (private) table serf_authn_schemes, defined in auth/auth.c, has
>>> been extended to have room for as many schemes as there are bits in
>>> serf__authn_scheme_t::type. Locking was added to serialize access,
>>> because new schemes can be registered at runtime.
>>>
>>> * Private callbacks were added to implement the user-visible
>>> authentication flow.
>>>
>>> * Two new public APIs take care of registering and unregistering
>>> authentication schemes. The latter is tricky because we don't track
>>> references to the schemes in contexts and connections, only in the
>>> serf_authn_schemes table; this issue is documented.
>>>
>>> * A number of tests were added for all this new stuff.
>>>
>>>
>>> Please take a look if you have time.
>>>
>>> svn diff --internal-diff \
>>> https://svn.apache.org/repos/asf/serf/trunk \
>>> https://svn.apache.org/repos/asf/serf/branches/user-defined-authn \
>>> | colordiff --color=always \
>>> | less -R
>>>
>>
>> There are a couple more changes on that branch now:
>>
>> * The parameters in the authentication challenge (headers
>> WWW-Authenticate and Proxy-Authenticate) are now parsed into a
>> dictionary, using the rules described in RFC 9110.
>>
>> * The Authentication-Info and Proxy-Authentication-Info response
>> headers are now handled and their values parsed as above.
>>
>> These two changes should make life much easier for authn scheme implementers. I decided to add this once I saw how many times we have copy-pasted not-quite-compliant parsers throughout the auth code...
>>
>> (Note that these changes do not stop implementers from doing whatever wrong-headed, non-compliant horror they can think of, as all the raw information and headers are still available in the callbacks.)
>
> Any takers? It's been a couple of weeks now. I'd sure like to get another pair of eyes on these changes before merging them to trunk, but I'd rather not have the branch fall victim to bitrot, either.
I'll bite - so I've tried it against some auth testing code we privately maintain for legacy hardware keys - and it certainly gets rid of a lot of the boilerplate/header parse code - without loss of functionality. And oddly it passes now an EAGAIN/EINTR edge case that always broke our tests.
Improvement overall.
Dw
Re: Thinking about MFA infrastructure
Posted by Branko Čibej <br...@apache.org>.
On 8. 7. 25 17:26, Branko Čibej wrote:
> On 26. 6. 25 08:16, Branko Čibej wrote:
>> On 24. 6. 25 07:52, Branko Čibej wrote:
>>> On 14. 6. 25 21:23, Branko Čibej wrote:
>>>> Hi all,
>>>>
>>>> I've been digging into Serf's support for various authentication
>>>> schemes and I notices something that looks like a bit of a limitation.
>>>>
>>>> Unless I'm much mistaken, there's space for only one authentication
>>>> baton in Serf's context. It would seem that this is rather a
>>>> blocker for implementing multi-factor authentication flows, for
>>>> example, Basic + OTP, where the server would first require basic
>>>> credentials and then, if those were correct, go on to issue an OTP
>>>> challenge.
>>>>
>>>> It seems to me that a simple solution for that would be to store an
>>>> authn baton per scheme, but I know on the close order of nothing
>>>> about the possible side effects.
>>>>
>>>> Yeah, I'm starting small, I have no wish to implement OAuth2 flow
>>>> any time soon. Still, a bit of insight from the knowledgeable would
>>>> be welcome.
>>>
>>>
>>> With r1926674, I think the user-defined-authn branch is ready for
>>> review. Here's a short summary of the changes on the branch:
>>>
>>> * The (private) struct serf__authn_scheme_t, defined in auth/auth.h,
>>> grew a number of new members to support user-defined schemes, among
>>> them five new user-visible callback functions. These, along with
>>> their interactions, are described in serf.h. The built-in
>>> authentication schemes don't need or use these new members.
>>>
>>> * The (private) table serf_authn_schemes, defined in auth/auth.c, has
>>> been extended to have room for as many schemes as there are bits in
>>> serf__authn_scheme_t::type. Locking was added to serialize access,
>>> because new schemes can be registered at runtime.
>>>
>>> * Private callbacks were added to implement the user-visible
>>> authentication flow.
>>>
>>> * Two new public APIs take care of registering and unregistering
>>> authentication schemes. The latter is tricky because we don't track
>>> references to the schemes in contexts and connections, only in the
>>> serf_authn_schemes table; this issue is documented.
>>>
>>> * A number of tests were added for all this new stuff.
>>>
>>>
>>> Please take a look if you have time.
>>>
>>> svn diff --internal-diff \
>>> https://svn.apache.org/repos/asf/serf/trunk \
>>> https://svn.apache.org/repos/asf/serf/branches/user-defined-authn \
>>> | colordiff --color=always \
>>> | less -R
>>>
>>
>> There are a couple more changes on that branch now:
>>
>> * The parameters in the authentication challenge (headers
>> WWW-Authenticate and Proxy-Authenticate) are now parsed into a
>> dictionary, using the rules described in RFC 9110.
>>
>> * The Authentication-Info and Proxy-Authentication-Info response
>> headers are now handled and their values parsed as above.
>>
>> These two changes should make life much easier for authn scheme
>> implementers. I decided to add this once I saw how many times we have
>> copy-pasted not-quite-compliant parsers throughout the auth code...
>>
>> (Note that these changes do not stop implementers from doing whatever
>> wrong-headed, non-compliant horror they can think of, as all the raw
>> information and headers are still available in the callbacks.)
>
>
> Any takers? It's been a couple of weeks now. I'd sure like to get
> another pair of eyes on these changes before merging them to trunk,
> but I'd rather not have the branch fall victim to bitrot, either.
After a month of waiting for feedback, I decided to merge this branch to
trunk in r1927627. All my testing on several platforms has not shown any
problems.
-- Brane