Radius provider

[Ondra Hujňák]

Hi,

I'm about to develop new provider for RADIUS protocol. I just made some stub and wanted to ask you guys to check the coding style.

My work is available at github repo https://github.com/hujon/sssd.git in branch rad.

So far it's just a really preliminary review which lacks support for RADIUS. I just wanted to know if I'm heading the right way.

Ondrej Hujnak

[Jakub Hrozek]

On Wed, Apr 10, 2013 at 08:35:51AM +0200, Ondra Hujňák wrote:
> Hi,
>
> I'm about to develop new provider for RADIUS protocol. I just made some stub and wanted to ask you guys to check the coding style.
>
> My work is available at github repo https://github.com/hujon/sssd.git in branch rad.
>
> So far it's just a really preliminary review which lacks support for RADIUS. I just wanted to know if I'm heading the right way.
>
> Ondrej Hujnak

Hi Ondra,

the stub looks OK, I just have a couple of style comments so far
(below).

But most importantly, do you have some kind of design document?
Anything that describes how the provider should behave and what the
functionality will be like? For instance I don't know which PAM tasks
are supported by RADIUS so I can't say if all the actions in
get_rad_ctx() should be supported or not.

As per the style:
 * Don't mix tabs and spaces. Follow the coding guidelines on
   http://www.freeipa.org/page/Coding_Style
 * Put a space after keywords ("if (" not "if(") and no space after
 * opening "(".
 * Using the new SSSDBG_* DEBUG levels is preferred over the old decimal
   levels
 * Please put a "copyright blob" on top of every new file. You can copy
   the blobs from other files but please check with your advisor who
   holds the copyright since you're doing the project as a thesis -- is
   it you or the university?

Also we have recently started to use new SSSD-specific error codes in
favor of errno. You can take a look at commits
233a3c6c48972b177e60d6ef4cecfacd3cf31659 or
dfd71fc92db940b2892cc996911cec03d7b6c52b

I would strongly prefer if the new provider used the new error codes.

When this feature is accepted upstream, it would be an experimental
feature I think. Can you modify the autoconf scripts so that this new
provider is only built when --enable-all-experimental-features is used?
Check out commit b2f9e5b7d553172401a340eb4a9c3abda6b5db43 to see how a
feature can be marked experimental.

[Dmitri Pal]

On 04/10/2013 05:40 AM, Jakub Hrozek wrote:
> On Wed, Apr 10, 2013 at 08:35:51AM +0200, Ondra Hujňák wrote:
> > Hi,
> >
> > I'm about to develop new provider for RADIUS protocol. I just made some stub and wanted to ask you guys to check the coding style.
> >
> > My work is available at github repo https://github.com/hujon/sssd.git in branch rad.
> >
> > So far it's just a really preliminary review which lacks support for RADIUS. I just wanted to know if I'm heading the right way.
> >
> > Ondrej Hujnak
>
> Hi Ondra,
>
> the stub looks OK, I just have a couple of style comments so far
> (below).
>
> But most importantly, do you have some kind of design document?

And even before that I want to start with requirements.
Can you please specify how you understand the requirements?
It would help us to guide you in the design phase and clarify what needs
to be accomplished.

Thanks
Dmitri
> Anything that describes how the provider should behave and what the
> functionality will be like? For instance I don't know which PAM tasks
> are supported by RADIUS so I can't say if all the actions in
> get_rad_ctx() should be supported or not.
>
> As per the style:
> Don't mix tabs and spaces. Follow the coding guidelines on
> http://www.freeipa.org/page/Coding_Style
> Put a space after keywords ("if (" not "if(") and no space after
> opening "(".
> Using the new SSSDBG_* DEBUG levels is preferred over the old decimal
> levels
> Please put a "copyright blob" on top of every new file. You can copy
> the blobs from other files but please check with your advisor who
> holds the copyright since you're doing the project as a thesis -- is
> it you or the university?
>
> Also we have recently started to use new SSSD-specific error codes in
> favor of errno. You can take a look at commits
> 233a3c6c48972b177e60d6ef4cecfacd3cf31659 or
> dfd71fc92db940b2892cc996911cec03d7b6c52b
>
> I would strongly prefer if the new provider used the new error codes.
>
> When this feature is accepted upstream, it would be an experimental
> feature I think. Can you modify the autoconf scripts so that this new
> provider is only built when --enable-all-experimental-features is used?
> Check out commit b2f9e5b7d553172401a340eb4a9c3abda6b5db43 to see how a
> feature can be marked experimental.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Ondra Hujňák]

10. 4. 2013 v 13:55, Dmitri Pal :
> On 04/10/2013 05:40 AM, Jakub Hrozek wrote:
> > On Wed, Apr 10, 2013 at 08:35:51AM +0200, Ondra Hujňák wrote:
> > > Hi,
> > >
> > > I'm about to develop new provider for RADIUS protocol. I just made some stub and wanted to ask you guys to check the coding style.
> > >
> > > My work is available at github repo https://github.com/hujon/sssd.git in branch rad.
> > >
> > > So far it's just a really preliminary review which lacks support for RADIUS. I just wanted to know if I'm heading the right way.
> > >
> > > Ondrej Hujnak
> >
> > Hi Ondra,
> >
> > the stub looks OK, I just have a couple of style comments so far
> > (below).
> >
> > But most importantly, do you have some kind of design document?
>
> And even before that I want to start with requirements.
> Can you please specify how you understand the requirements?
> It would help us to guide you in the design phase and clarify what needs
> to be accomplished.
>
> Thanks
> Dmitri

Well, the provider is meant to communicate with RADIUS server,
so basic two requirements are SSSD and RADIUS server. Because
there is no way how to obtain users from RADIUS (AFAIK) there
have to be some id provider (such as LDAP).

Is that what you meant?

About the design document, I am afraid I do not have it right now,
but I'll do it and post it here asap.
> > Anything that describes how the provider should behave and what the
> > functionality will be like? For instance I don't know which PAM tasks
> > are supported by RADIUS so I can't say if all the actions in
> > get_rad_ctx() should be supported or not.
> >
> > As per the style:
> > Don't mix tabs and spaces. Follow the coding guidelines on
> > http://www.freeipa.org/page/Coding_Style
> > Put a space after keywords ("if (" not "if(") and no space after
> > opening "(".
> > Using the new SSSDBG_* DEBUG levels is preferred over the old decimal
> > levels
> > Please put a "copyright blob" on top of every new file. You can copy
> > the blobs from other files but please check with your advisor who
> > holds the copyright since you're doing the project as a thesis -- is
> > it you or the university?
> >
> > Also we have recently started to use new SSSD-specific error codes in
> > favor of errno. You can take a look at commits
> > 233a3c6c48972b177e60d6ef4cecfacd3cf31659 or
> > dfd71fc92db940b2892cc996911cec03d7b6c52b
> >
> > I would strongly prefer if the new provider used the new error codes.
> >
> > When this feature is accepted upstream, it would be an experimental
> > feature I think. Can you modify the autoconf scripts so that this new
> > provider is only built when --enable-all-experimental-features is used?
> > Check out commit b2f9e5b7d553172401a340eb4a9c3abda6b5db43 to see how a
> > feature can be marked experimental.
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> -- 
> Thank you,
> Dmitri Pal
>
> Sr. Engineering Manager for IdM portfolio
> Red Hat Inc.
>
> Looking to carve out IT costs?
> www.redhat.com/carveoutcosts/
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Thank you for the comments,
Ondrej Hujnak

[Dmitri Pal]

On 04/10/2013 08:45 AM, Ondra Hujňák wrote:
> 2013 v 13:55, Dmitri Pal :
> > On 04/10/2013 05:40 AM, Jakub Hrozek wrote:
> > > On Wed, Apr 10, 2013 at 08:35:51AM +0200, Ondra Hujňák wrote:
> > > > Hi,
> > > >
> > > > I'm about to develop new provider for RADIUS protocol. I just made some stub and wanted to ask you guys to check the coding style.
> > > >
> > > > My work is available at github repo https://github.com/hujon/sssd.git in branch rad.
> > > >
> > > > So far it's just a really preliminary review which lacks support for RADIUS. I just wanted to know if I'm heading the right way.
> > > >
> > > > Ondrej Hujnak
> > >
> > > Hi Ondra,
> > >
> > > the stub looks OK, I just have a couple of style comments so far
> > > (below).
> > >
> > > But most importantly, do you have some kind of design document?
> >
> > And even before that I want to start with requirements.
> > Can you please specify how you understand the requirements?
> > It would help us to guide you in the design phase and clarify what needs
> > to be accomplished.
> >
> > Thanks
> > Dmitri
>
> Well, the provider is meant to communicate with RADIUS server,
> so basic two requirements are SSSD and RADIUS server. Because
> there is no way how to obtain users from RADIUS (AFAIK) there
> have to be some id provider (such as LDAP).
>
> Is that what you meant?

I think the caveat it that the requirement is to be able to authenticate
as a RADIUS user and then to be able to optionally map to a single ldap
or local user. At least that was one of the use cases. I know it is not
the best practice but it is something that people have been doing in the
past and they need this functionality still until they can move to some
alternative solution which is more secure.
> About the design document, I am afraid I do not have it right now,
> but I'll do it and post it here asap.
> > > Anything that describes how the provider should behave and what the
> > > functionality will be like? For instance I don't know which PAM tasks
> > > are supported by RADIUS so I can't say if all the actions in
> > > get_rad_ctx() should be supported or not.
> > >
> > > As per the style:
> > > Don't mix tabs and spaces. Follow the coding guidelines on
> > > http://www.freeipa.org/page/Coding_Style
> > > Put a space after keywords ("if (" not "if(") and no space after
> > > opening "(".
> > > Using the new SSSDBG_* DEBUG levels is preferred over the old decimal
> > > levels
> > > Please put a "copyright blob" on top of every new file. You can copy
> > > the blobs from other files but please check with your advisor who
> > > holds the copyright since you're doing the project as a thesis -- is
> > > it you or the university?
> > >
> > > Also we have recently started to use new SSSD-specific error codes in
> > > favor of errno. You can take a look at commits
> > > 233a3c6c48972b177e60d6ef4cecfacd3cf31659 or
> > > dfd71fc92db940b2892cc996911cec03d7b6c52b
> > >
> > > I would strongly prefer if the new provider used the new error codes.
> > >
> > > When this feature is accepted upstream, it would be an experimental
> > > feature I think. Can you modify the autoconf scripts so that this new
> > > provider is only built when --enable-all-experimental-features is used?
> > > Check out commit b2f9e5b7d553172401a340eb4a9c3abda6b5db43 to see how a
> > > feature can be marked experimental.
> > > _______________________________________________
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > -- 
> > Thank you,
> > Dmitri Pal
> >
> > Sr. Engineering Manager for IdM portfolio
> > Red Hat Inc.
> >
> > Looking to carve out IT costs?
> > www.redhat.com/carveoutcosts/
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Thank you for the comments,
> Ondrej Hujnak
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Ondra Hujňák]

10. 4. 2013 v 16:01, Dmitri Pal :
> On 04/10/2013 08:45 AM, Ondra Hujňák wrote:
> > 2013 v 13:55, Dmitri Pal :
> > > On 04/10/2013 05:40 AM, Jakub Hrozek wrote:
> > > > On Wed, Apr 10, 2013 at 08:35:51AM +0200, Ondra Hujňák wrote:
> > > > > Hi,
> > > > >
> > > > > I'm about to develop new provider for RADIUS protocol. I just made some stub and wanted to ask you guys to check the coding style.
> > > > >
> > > > > My work is available at github repo https://github.com/hujon/sssd.git in branch rad.
> > > > >
> > > > > So far it's just a really preliminary review which lacks support for RADIUS. I just wanted to know if I'm heading the right way.
> > > > >
> > > > > Ondrej Hujnak
> > > >
> > > > Hi Ondra,
> > > >
> > > > the stub looks OK, I just have a couple of style comments so far
> > > > (below).
> > > >
> > > > But most importantly, do you have some kind of design document?
> > >
> > > And even before that I want to start with requirements.
> > > Can you please specify how you understand the requirements?
> > > It would help us to guide you in the design phase and clarify what needs
> > > to be accomplished.
> > >
> > > Thanks
> > > Dmitri
> >
> > Well, the provider is meant to communicate with RADIUS server,
> > so basic two requirements are SSSD and RADIUS server. Because
> > there is no way how to obtain users from RADIUS (AFAIK) there
> > have to be some id provider (such as LDAP).
> >
> > Is that what you meant?
>
> I think the caveat it that the requirement is to be able to authenticate
> as a RADIUS user and then to be able to optionally map to a single ldap
> or local user. At least that was one of the use cases. I know it is not
> the best practice but it is something that people have been doing in the
> past and they need this functionality still until they can move to some
> alternative solution which is more secure.

I'm not sure if I truly understand you. Could you be a little more specific about
your use case?
> > About the design document, I am afraid I do not have it right now,
> > but I'll do it and post it here asap.
> > > > Anything that describes how the provider should behave and what the
> > > > functionality will be like? For instance I don't know which PAM tasks
> > > > are supported by RADIUS so I can't say if all the actions in
> > > > get_rad_ctx() should be supported or not.

Some description is attached to this email.
> > > > As per the style:
> > > > Don't mix tabs and spaces. Follow the coding guidelines on
> > > >
> > > > http://www.freeipa.org/page/Coding_Style
> > > > Put a space after keywords ("if (" not "if(") and no space after
> > > > opening "(".
> > > > Using the new SSSDBG_* DEBUG levels is preferred over the old decimal
> > > >
> > > > levels
> > > > Please put a "copyright blob" on top of every new file. You can copy
> > > >
> > > > the blobs from other files but please check with your advisor who
> > > >  holds the copyright since you're doing the project as a thesis -- is
> > > >  it you or the university?
> > > >
> > > > Also we have recently started to use new SSSD-specific error codes in
> > > > favor of errno. You can take a look at commits
> > > > 233a3c6c48972b177e60d6ef4cecfacd3cf31659 or
> > > > dfd71fc92db940b2892cc996911cec03d7b6c52b
> > > >
> > > > I would strongly prefer if the new provider used the new error codes.
> > > >
> > > > When this feature is accepted upstream, it would be an experimental
> > > > feature I think. Can you modify the autoconf scripts so that this new
> > > > provider is only built when --enable-all-experimental-features is used?
> > > > Check out commit b2f9e5b7d553172401a340eb4a9c3abda6b5db43 to see how a
> > > > feature can be marked experimental.
> > > > _______________________________________________
> > > > sssd-devel mailing list
> > > > [e-mail]
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > > -- 
> > > Thank you,
> > > Dmitri Pal
> > >
> > > Sr. Engineering Manager for IdM portfolio
> > > Red Hat Inc.
> > >
> > > Looking to carve out IT costs?
> > > www.redhat.com/carveoutcosts/
> > >
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > Thank you for the comments,
> > Ondrej Hujnak
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> -- 
> Thank you,
> Dmitri Pal
>
> Sr. Engineering Manager for IdM portfolio
> Red Hat Inc.
>
> Looking to carve out IT costs?
> www.redhat.com/carveoutcosts/
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

I tried to fix my style. There are some new commits in my github repo:
https://github.com/hujon/sssd.git branch rad

Ondrej Hujnak

[Dmitri Pal]

On 04/16/2013 09:03 AM, Ondra Hujňák wrote:
> 2013 v 16:01, Dmitri Pal :
> > On 04/10/2013 08:45 AM, Ondra Hujňák wrote:
> > > 2013 v 13:55, Dmitri Pal :
> > > > On 04/10/2013 05:40 AM, Jakub Hrozek wrote:
> > > > > On Wed, Apr 10, 2013 at 08:35:51AM +0200, Ondra Hujňák wrote:
> > > > > > Hi,
> > > > > >
> > > > > > I'm about to develop new provider for RADIUS protocol. I just made some stub and wanted to ask you guys to check the coding style.
> > > > > >
> > > > > > My work is available at github repo https://github.com/hujon/sssd.git in branch rad.
> > > > > >
> > > > > > So far it's just a really preliminary review which lacks support for RADIUS. I just wanted to know if I'm heading the right way.
> > > > > >
> > > > > > Ondrej Hujnak
> > > > >
> > > > > Hi Ondra,
> > > > >
> > > > > the stub looks OK, I just have a couple of style comments so far
> > > > > (below).
> > > > >
> > > > > But most importantly, do you have some kind of design document?
> > > >
> > > > And even before that I want to start with requirements.
> > > > Can you please specify how you understand the requirements?
> > > > It would help us to guide you in the design phase and clarify what needs
> > > > to be accomplished.
> > > >
> > > > Thanks
> > > > Dmitri
> > >
> > > Well, the provider is meant to communicate with RADIUS server,
> > > so basic two requirements are SSSD and RADIUS server. Because
> > > there is no way how to obtain users from RADIUS (AFAIK) there
> > > have to be some id provider (such as LDAP).
> > >
> > > Is that what you meant?
> >
> > I think the caveat it that the requirement is to be able to authenticate
> > as a RADIUS user and then to be able to optionally map to a single ldap
> > or local user. At least that was one of the use cases. I know it is not
> > the best practice but it is something that people have been doing in the
> > past and they need this functionality still until they can move to some
> > alternative solution which is more secure.
>
> I'm not sure if I truly understand you. Could you be a little more specific about
> your use case?

RADIUS is just for authentication not for identity so the identity would
come from the local files or LDAP or something else.
In general, recommended, case the user who authenticated would be
identified as the same user, i.e. you authenticated as ondrah and the
user is ondrah.
In some cases however this is not possible. Think about an appliance.
This appliance is given by service provider to the client. The client
authenticates to the device and the device uses RADIUS to talk to the
service provider. Service provider is Ok to expose RADIUS outside the
firewall but he is not OK to expose LDAP. So the device would
authenticate user over radius and then map user to the local user.
Usually all authenticated users are mapped to a single local user. It is
the bad practice because you can't audit correctly the user activity on
the box but it all depends on what the appliance does and it might not
be that big of an issue.

This is the use case. HTH.
> > > About the design document, I am afraid I do not have it right now,
> > > but I'll do it and post it here asap.
> > > > > Anything that describes how the provider should behave and what the
> > > > > functionality will be like? For instance I don't know which PAM tasks
> > > > > are supported by RADIUS so I can't say if all the actions in
> > > > > get_rad_ctx() should be supported or not.
>
> Some description is attached to this email.
> > > > > As per the style:
> > > > > Don't mix tabs and spaces. Follow the coding guidelines on
> > > > >
> > > > > http://www.freeipa.org/page/Coding_Style
> > > > > Put a space after keywords ("if (" not "if(") and no space after
> > > > > opening "(".
> > > > > Using the new SSSDBG_* DEBUG levels is preferred over the old decimal
> > > > >
> > > > > levels
> > > > > Please put a "copyright blob" on top of every new file. You can copy
> > > > >
> > > > > the blobs from other files but please check with your advisor who
> > > > >  holds the copyright since you're doing the project as a thesis -- is
> > > > >  it you or the university?
> > > > >
> > > > > Also we have recently started to use new SSSD-specific error codes in
> > > > > favor of errno. You can take a look at commits
> > > > > 233a3c6c48972b177e60d6ef4cecfacd3cf31659 or
> > > > > dfd71fc92db940b2892cc996911cec03d7b6c52b
> > > > >
> > > > > I would strongly prefer if the new provider used the new error codes.
> > > > >
> > > > > When this feature is accepted upstream, it would be an experimental
> > > > > feature I think. Can you modify the autoconf scripts so that this new
> > > > > provider is only built when --enable-all-experimental-features is used?
> > > > > Check out commit b2f9e5b7d553172401a340eb4a9c3abda6b5db43 to see how a
> > > > > feature can be marked experimental.
> > > > > _______________________________________________
> > > > > sssd-devel mailing list
> > > > > [e-mail]
> > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > >
> > > > -- 
> > > > Thank you,
> > > > Dmitri Pal
> > > >
> > > > Sr. Engineering Manager for IdM portfolio
> > > > Red Hat Inc.
> > > >
> > > > Looking to carve out IT costs?
> > > > www.redhat.com/carveoutcosts/
> > > >
> > > > sssd-devel mailing list
> > > > [e-mail]
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > > Thank you for the comments,
> > > Ondrej Hujnak
> > >
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > -- 
> > Thank you,
> > Dmitri Pal
> >
> > Sr. Engineering Manager for IdM portfolio
> > Red Hat Inc.
> >
> > Looking to carve out IT costs?
> > www.redhat.com/carveoutcosts/
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> I tried to fix my style. There are some new commits in my github repo:
> https://github.com/hujon/sssd.git branch rad
>
> Ondrej Hujnak
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Simo Sorce]

On Wed, 2013-04-10 at 11:40 +0200, Jakub Hrozek wrote:
> On Wed, Apr 10, 2013 at 08:35:51AM +0200, Ondra Hujňák wrote:
> > Hi,
> >
> > I'm about to develop new provider for RADIUS protocol. I just made some stub and wanted to ask you guys to check the coding style.
> >
> > My work is available at github repo https://github.com/hujon/sssd.git in branch rad.
> >
> > So far it's just a really preliminary review which lacks support for RADIUS. I just wanted to know if I'm heading the right way.
> >
> > Ondrej Hujnak
>
> Hi Ondra,
>
> the stub looks OK, I just have a couple of style comments so far
> (below).
>
> But most importantly, do you have some kind of design document?
> Anything that describes how the provider should behave and what the
> functionality will be like? For instance I don't know which PAM tasks
> are supported by RADIUS so I can't say if all the actions in
> get_rad_ctx() should be supported or not.
>
> As per the style:
> Don't mix tabs and spaces. Follow the coding guidelines on
> http://www.freeipa.org/page/Coding_Style
> Put a space after keywords ("if (" not "if(") and no space after
> opening "(".
> Using the new SSSDBG_* DEBUG levels is preferred over the old decimal
> levels
> Please put a "copyright blob" on top of every new file. You can copy
> the blobs from other files but please check with your advisor who
> holds the copyright since you're doing the project as a thesis -- is
> it you or the university?
>
> Also we have recently started to use new SSSD-specific error codes in
> favor of errno. You can take a look at commits
> 233a3c6c48972b177e60d6ef4cecfacd3cf31659 or
> dfd71fc92db940b2892cc996911cec03d7b6c52b
>
> I would strongly prefer if the new provider used the new error codes.
>
> When this feature is accepted upstream, it would be an experimental
> feature I think. Can you modify the autoconf scripts so that this new
> provider is only built when --enable-all-experimental-features is used?
> Check out commit b2f9e5b7d553172401a340eb4a9c3abda6b5db43 to see how a
> feature can be marked experimental.

Also please always use 4 spaces indentation and do not indent 'case' in
'switch' statements. Do not use C++ style comments (// comment).

Try not to create too many header files, you do not need one header file
for each .c file unless you really need to keep them separate.

Usually for small modules I would create at most a radius_common.h and a
radius_private.h files and the latter only if you really need a
differentiation.

Simo.