Negotiate

[lublak]

fixes: #378

[jem-davies]

Great - so we are adding a bool config field to the http client to enable a few more auth/protocols - this is great, I think that this is an improvement no doubt.

A few things:

Couldn't we simplify spnego_kerberos.go significantly?

For instance we are loading the krb5.conf - we have some fallback mechanisms if the env var KRB5_CONFIG hasn't been supplied, and using file, err := os.Open and config.NewFromReader(file) - Couldn't we just create a Bento config field, say krb5_config_filepath and then use config.Load(filepath)?

As well here, you then have a large implementation of RoundTrip - A lot of this seems that it would be better if it was provided by the dependency - but that only provides a Client and not a Transport ...

Documentation / New Config fields

Most user configuration is supplied to Bento via the config fields - rather than make use of Env Vars being set - we should be using config fields. When config fields are used and are given adequate documentation it helps to keep Bento maintainable and usable.

I think that we should look at including something more descriptive - a config object field rather than a single bool?

Kind of going back to the point around loading the config - currently with the fallback - there is no documentation to say, set this Env Var, and we will also fallback to xyz on windows, mac etc. Let's keep it simple and expose a config field to set for the krb5.conf (we could allow both a string in the yaml config or a filepath, similar to tls) but if either of those fail then we report error back to the user.

spnego_windows.go

Admittedly I haven't looked at this one much - but why do we need a separate impl for windows? Can't jcmturner/gokrb5 do what we need it to do? It does mention Active Directory here - and also it's written in pure go?

Tests

We could do with some tests I think - I have been taking a look at this spnego and kerberos and have a working docker setup - would you be able to include an integration test using the docker image: gcavalcante8808/krb5-server ? If not it's no big deal - I will look to add one but want to address the above first I think?

[lublak]

Thanks for the review. I only published the implementation in this form after careful consideration.
Here are my thoughts and opinions:

Couldn't we simplify spnego_kerberos.go significantly?

For instance we are loading the krb5.conf - we have some fallback mechanisms if the env var KRB5_CONFIG hasn't been supplied, and using file, err := os.Open and config.NewFromReader(file) - Couldn't we just create a Bento config field, say krb5_config_filepath and then use config.Load(filepath)?

The reason I decided against a configuration field is more or less to stick to the standard implementation of all applications.
KRB5_CONFIG is seen here as a standard variable and configured on the system. But only if the standard paths for each system are not adhered to.
Nevertheless, I considered making it possible as a configuration field, perhaps even as an optional field. However, the reason I decided against it is the difference between a Windows system and a Unix-based system. More on this later in the comment sspi.
There is no such configuration file in SSPI. This would therefore be a field that is more or less ignored.
However, since we already have a standard with ‘fixed file paths’ and environment variables for Keberos, it would probably be more confusing than useful.
One more piece of information here. Basically, I actually wanted to support gssapi for Unix. However, its usability in Go is very limited, and it would have been necessary to work on CGO here. That is why I opted for the very extensive keberos implementation, which basically corresponds to the standard on the Unix side.

As well here, you then have a large implementation of RoundTrip - A lot of this seems that it would be better if it was provided by the dependency - but that only provides a Client and not a Transport ...

Exactly. There is no RoundTrip dependency here. There was an issue about this at one point, but it was closed and they just said that you have to implement it yourself.

Documentation / New Config fields

Most user configuration is supplied to Bento via the config fields - rather than make use of Env Vars being set - we should be using config fields. When config fields are used and are given adequate documentation it helps to keep Bento maintainable and usable.

I think that we should look at including something more descriptive - a config object field rather than a single bool?
Kind of going back to the point around loading the config - currently with the fallback - there is no documentation to say, set this Env Var, and we will also fallback to xyz on windows, mac etc. Let's keep it simple and expose a config field to set for the krb5.conf (we could allow both a string in the yaml config or a filepath, similar to tls) but if either of those fail then we report error back to the user.

Here I must say that I have adhered to the standard of most client solutions. The most notable example here would be ‘curl’.
https://curl.se/docs/manpage.html#--negotiate
If we want to have an optional field on the linux side for the configuration file (but please make it optional), then a structure would of course be useful here.

spnego_windows.go

Admittedly I haven't looked at this one much - but why do we need a separate impl for windows? Can't jcmturner/gokrb5 do what we need it to do? It does mention Active Directory here - and also it's written in pure go?

So I just have to say: Please don't.
I myself use sspi in the Windows world, and it is also included in Windows systems by default. In theory, it is the equivalent of gssapi in Linux. A computer that exists in Active Directory with a domain user can automatically authenticate to the server without having to perform a keberos setup beforehand.
I must also say that I first developed the sspi implementation on my side, but then followed suit on the Unix side for bento. This was so that bento would have the full range of functions on all platforms.

Tests

We could do with some tests I think - I have been taking a look at this spnego and kerberos and have a working docker setup - would you be able to include an integration test using the docker image: gcavalcante8808/krb5-server ? If not it's no big deal - I will look to add one but want to address the above first I think?

Of course we can do that. However, I believe that we can only add Keberos to the automatic tests here. It should be noted that I already have the Windows site up and running productively and have not encountered any problems so far.
But I think it will take a lot of time on my side. I have no idea how to create a docker based test on GitHub :smile

[jem-davies]

The reason I decided against a configuration field is more or less to stick to the standard implementation of all applications.

...

The most notable example here would be ‘curl’.

OK - I can see that curl, is very similar to what we have here, a single boolean setting --negotiate.

But on the man page for curl - it does go into then detail regarding:

"This option requires a library built with GSS-API or SSPI support. Use --version to see if your curl supports GSS-API/SSPI or SPNEGO."

Which I can see that when I do curl --version I get:

...
Features:  ... GSS-API, ..., Kerberos, ..., SPNEGO

With this PR don't we need to document that Bento supports Kerberos / sspi / ntlm? This just currently isn't mentioned is it.

Wouldn't altering PR's negotiate bool field to be an object field, where the sub-fields were configuration options for kerberos / sspi, etc. be helpful for users & maintainers? There you could have the field krb5_config which could describe the behaviour regarding the ENV VAR default and the fallback mechanisms?

Isn't this how it works 'negotiate' is SPNEGO and that then will select the actual authentication protocol - Kerberos / ntlm / sspi ... ?

Seems like we need to make this an object field, so that we can extend it in the future with more auth protocols & provide documentation for each of the auth protocols bento can be configured with?

With sspi - there is no config to supply there - how about a Bento config option negotiate.sspi_enabled -- Then there we can document we support sspi?

---

So I just have to say: Please don't.
I myself use sspi in the Windows world, and it is also included in Windows systems by default. In theory, it is the equivalent of gssapi in Linux. A computer that exists in Active Directory with a domain user can automatically authenticate to the server without having to perform a keberos setup beforehand.

My questions here regarding spnego_windows.go - why do we need both, is it because you originally developed with windows in mind for your own use-case and then used the other library to implement for unix system?

---

It should be noted that I already have the Windows site up and running productively and have not encountered any problems so far.

Ok sounds good, I can convert my set-up for a kerberos int. test then and perhaps we can leave windows as-is.

---

I think there wasn't much info for the issue & PR, all we had was:

PR Title: Negotiate Desc: fixes #378

In the issue we have a link to https://github.com/alexbrainman/sspi/blob/master/negotiate/http_test.go

And then coupled with the lack of documentation & tests in the PR itself has meant more reading for me. (But probably would have to do it anyway...)

[lublak]

With this PR don't we need to document that Bento supports Kerberos / sspi / ntlm? This just currently isn't mentioned is it.

Wouldn't altering PR's negotiate bool field to be an object field, where the sub-fields were configuration options for kerberos / sspi, etc. be helpful for users & maintainers? There you could have the field krb5_config which could describe the behaviour regarding the ENV VAR default and the fallback mechanisms?

Isn't this how it works 'negotiate' is SPNEGO and that then will select the actual authentication protocol - Kerberos / ntlm / sspi ... ?

Seems like we need to make this an object field, so that we can extend it in the future with more auth protocols & provide documentation for each of the auth protocols bento can be configured with?

With sspi - there is no config to supply there - how about a Bento config option negotiate.sspi_enabled -- Then there we can document we support sspi?

Okay thats a good idea.
I could change the pull request to an object and also support manuall kerberos for windows

negotiate:
    with: sspi | keberos | none (sspi only supported on windows, defaults to keberos?)
    krb5_config: defaults to env default file

Another idea is to support also gssapi (i need to figure out how to use it): https://github.com/jcmturner/gokrb5/tree/master/gssapi

And for ntlm: https://github.com/Azure/go-ntlmssp

negotiate:
    with: sspi | gssapi | keberos | ntlm | none (sspi only supported on windows, defaults to gssapi)
    krb5_config: defaults to env default file

So I just have to say: Please don't.
I myself use sspi in the Windows world, and it is also included in Windows systems by default. In theory, it is the equivalent of gssapi in Linux. A computer that exists in Active Directory with a domain user can automatically authenticate to the server without having to perform a keberos setup beforehand.

My questions here regarding spnego_windows.go - why do we need both, is it because you originally developed with windows in mind for your own use-case and then used the other library to implement for unix system?

It should be noted that I already have the Windows site up and running productively and have not encountered any problems so far.

Yes, I initially incorporated sspi here primarily out of my own internal interest. For the sake of completeness and, of course, to improve bento here, I subsequently implemented the Linux page as well.

Ok sounds good, I can convert my set-up for a kerberos int. test then and perhaps we can leave windows as-is.

I think there wasn't much info for the issue & PR, all we had was:

PR Title: Negotiate Desc: fixes #378

In the issue we have a link to https://github.com/alexbrainman/sspi/blob/master/negotiate/http_test.go

And then coupled with the lack of documentation & tests in the PR itself has meant more reading for me. (But probably would have to do it anyway...)

Yes, at the time I created this issue, I really didn't have any experience. I had only used sspi at different levels.
In general, perhaps some documentation should be added to the bento page.

[lublak]

@jem-davies I think that's the last message for now. If you give me the go-ahead, I'll start working on the changes.

[jem-davies]

@jem-davies I think that's the last message for now. If you give me the go-ahead, I'll start working on the changes.

Ok great thanks again!

Yeah I think my main concern was that the auth mechanism options weren't documented - so those changes you propose will rectify that.

I think that even if we just had a list of strings (string enum field) for negotiate like:

negotiate: 
  - kerberos
  - ... 

Then for each of the elements some documentation that explains how to set that up (i.e. with kerberos which env var to set).

After you explained about curl being the way it is about kerberos - we would be the same as that - so that is ok to leave out any krb5_config fields - again am not that fussed about that in particular so up to you.

We can tackle the other gssapi etc. in some follow-up prs - just add the option to the enum?

[lublak]

@jem-davies i know i know ... its another message (the last message was not the last message 😅 )

okay im fine with this! First i will documentate it. So we can to new stuff later.

Currently i need to think about the enable/disable mechanism.
Most of the time you want to just enable all auth protols and let it auto decide: spnego

So spnego is a protocol which wraps keberos or ntlm (or other procols from sspi/gssapi).
sspi and gssapi has implementation for ntlm keberos (mostly, sometimes more)

I implemented spnego myself for sspi and use spnego from the keberos lib.

I have to admit, this topic is giving me a bit of a headache.

deb packages for libgssapi

So correct would be (which has some issues just to have better documentations?):

negotiate:
  with:
    - sspi (if sspi not supported auto fall back to internal?)
    - gssapi (currently not supported, mostly keberos exists on linux)
    - internal (uses pure golang implementations)
  protocols: (can only be used with internal or sspi; gssapi uses its own spnego handler? Or should we use our own spnego implementation)
    - kerberos
    - ntlm (which is currently only not supported only on windows via sspi; i dont even now if it makes sense to support it more, its more like a deprecated protocol)

Support only keberos and ntlm for now?

• use own spnego (ntlm/keberos) for all "with" implementations (sspi/gssapi(if it will get implemented not really useful currently/internal)

I could also strip ntlm for now. I mean ... its a deprecated protocol.
I currently use the spnego -> sspi -> keberos route
The other route which makes sense would be spnego -> internal (should be mostly the same as gssapi with keberos) -> keberos route.

An alternative approach is: just use negotiation and let spnego decide.
So currently this would be
own spnego implementation + sspi
own spnego implementation+ internal keberos
In the feature? If needed
Gssapi spnego

(This is my current approach)

But you say: it should be a string enum to be documented. Which then can be cofigurated and has per auth documentation.

[lublak]

So to round up:

So spnego fallback is ntlm.
Negotiate header is mostly keberos
Can it be other protocols? Or does it need also some implementation like ntlm.

Option 1: just keep the current solution and improve the documentation

• has not headache
• gssapi could have its own spnego implementation in the feature (if needed?)
• all documentations are on one flag? Not quit nice?

Option 2: strip out the ntlm implementation for now and use a standalone spnego implementation

• limits gssapi in the feature (possibly? If all other protocols used to negotiate a header it should also work?)
• remove a feature from the sspi implementation
• has better documentation for each auth
• is a bit more complex to setup

Option 3: keep ntlm with the documentation: only works with sspi for now

• same as option 2 but keeps the ntlm sspi implementation
• discrepancy between different "targets"

[lublak]

Lets go with this rfc:

https://datatracker.ietf.org/doc/html/rfc4559

I will also now include ntlm to be complete

And this configuration:

negotiate: (spnego)
  api: (string enum) // should we throw an exception if we use sspi not on windows? Or should we fallback to internal?
    - sspi
    - gssapi (f someone needs it it could be implemented?)
    - internal (uses pure golang implementations)
  krb5: or keberos:? should we include the version number
    config: defaults to env default file
  ntlm:
    domain:
    user:
    password: