[bugfix] Added APOptions []int in client.GSSAPIBindRequest(...) and client.InitSecContext(...), fixes #536 #537

p0dalirius · 2024-11-14T15:07:05Z

Fixing Kerberos authentication due to missing APOption "MutualRequired"

fe5ed31 Added APOptions []int in client.GSSAPIBindRequest(...) and client.InitSecContext(...), fixes [bug] Could not make a working code example on connecting to LDAP with Kerberos from a linux machine to a Windows Server #536

Overview of the fix

In `client.InitSecContext(...)`

I have changed the client.InitSecContext(...) prototype from:

func (client *Client) InitSecContext(target string, input []byte) ([]byte, bool, error)

to:

func (client *Client) InitSecContext(target string, input []byte, APOptions []int) ([]byte, bool, error)

to be able to pass specific flags through the APOptions []int to the call to spnego.NewKRB5TokenAPREQ(client.Client, tkt, ekey, gssapiFlags, APOptions)

In `client.GSSAPIBind(...)`

I have left this function as it was, so it can be used in the generic case of GSSAPI authentication without the need to pass specific AP Options flags in parameters.

In `client.GSSAPIBindRequest(...)`

I have changed the client.GSSAPIBindRequest(...) prototype from:

func (l *Conn) GSSAPIBindRequest(client GSSAPIClient, req *GSSAPIBindRequest) error

to

func (l *Conn) GSSAPIBindRequest(client GSSAPIClient, req *GSSAPIBindRequest, APOptions []int) error

Example of a working code for Kerberos authentication

package main

import (
	"encoding/hex"
	"fmt"
	"log"
	"strings"
	"time"

	"github.com/go-ldap/ldap/v3"
	"github.com/jcmturner/gokrb5/v8/iana/flags"
	"github.com/go-ldap/ldap/v3/gssapi"
	"github.com/jcmturner/gokrb5/v8/client"
	"github.com/jcmturner/gokrb5/v8/config"
)

func main() {
	fqdnLDAPHost := "SRV-DC01.lab.local"
	baseDN := "DC=LAB,DC=local"

	realm := "lab.local"
	realm = strings.ToUpper(realm)
	// This is always in uppercase, if not we get the error:
	// error performing GSSAPI bind: [Root cause: KRBMessage_Handling_Error]
	// | KRBMessage_Handling_Error: AS Exchange Error: AS_REP is not valid or client password/keytab incorrect
	// |  | KRBMessage_Handling_Error: CRealm in response does not match what was requested.
	// |  |  | Requested: lab.local;
	// |  |  | Reply: lab.local
	// | 2024/10/08 15:36:16 error querying AD: LDAP Result Code 1 "Operations Error": 000004DC: LdapErr: DSID-0C090A5C,
	// | comment: In order to perform this operation a successful bind must be completed on the connection., data 0, v4563

	username := "Administrator"
	// error performing GSSAPI bind: [Root cause: KDC_Error] KDC_Error: AS Exchange Error: kerberos error response from KDC:
	// KRB Error: (6) KDC_ERR_C_PRINCIPAL_UNKNOWN Client not found in Kerberos database
	// KDC_ERR_C_PRINCIPAL_UNKNOWN (error code 6) for these means that the domain controller to which the request
	// was made does not host the account and the client should choose a different domain controller.
	// src: https://learn.microsoft.com/en-us/troubleshoot/windows-server/certificates-and-public-key-infrastructure-pki/kdc-err-c-principal-unknown-s4u2self-request
	// ==> This means this username does not exist

	password := "Admin123!"

	servicePrincipalName := fmt.Sprintf("ldap/%s", fqdnLDAPHost)

	krb5Conf := config.New()
	// LibDefaults
	krb5Conf.LibDefaults.AllowWeakCrypto = true
	krb5Conf.LibDefaults.DefaultRealm = realm
	krb5Conf.LibDefaults.DNSLookupRealm = false
	krb5Conf.LibDefaults.DNSLookupKDC = false
	krb5Conf.LibDefaults.TicketLifetime = time.Duration(24) * time.Hour
	krb5Conf.LibDefaults.RenewLifetime = time.Duration(24*7) * time.Hour
	krb5Conf.LibDefaults.Forwardable = true
	krb5Conf.LibDefaults.Proxiable = true
	krb5Conf.LibDefaults.RDNS = false
	krb5Conf.LibDefaults.UDPPreferenceLimit = 1 // Force use of tcp
	krb5Conf.LibDefaults.DefaultTGSEnctypes = []string{"aes256-cts-hmac-sha1-96", "aes128-cts-hmac-sha1-96", "arcfour-hmac-md5"}
	krb5Conf.LibDefaults.DefaultTktEnctypes = []string{"aes256-cts-hmac-sha1-96", "aes128-cts-hmac-sha1-96", "arcfour-hmac-md5"}
	krb5Conf.LibDefaults.PermittedEnctypes = []string{"aes256-cts-hmac-sha1-96", "aes128-cts-hmac-sha1-96", "arcfour-hmac-md5"}
	krb5Conf.LibDefaults.PermittedEnctypeIDs = []int32{18, 17, 23}
	krb5Conf.LibDefaults.DefaultTGSEnctypeIDs = []int32{18, 17, 23}
	krb5Conf.LibDefaults.DefaultTktEnctypeIDs = []int32{18, 17, 23}
	krb5Conf.LibDefaults.PreferredPreauthTypes = []int{18, 17, 23}

	// Realms
	krb5Conf.Realms = append(krb5Conf.Realms, config.Realm{
		Realm:         realm,
		AdminServer:   []string{fqdnLDAPHost},
		DefaultDomain: realm,
		KDC:           []string{fmt.Sprintf("%s:88", fqdnLDAPHost)},
		KPasswdServer: []string{fmt.Sprintf("%s:464", fqdnLDAPHost)},
		MasterKDC:     []string{fqdnLDAPHost},
	})

	// Domain Realm
	krb5Conf.DomainRealm[strings.ToLower(realm)] = realm
	krb5Conf.DomainRealm[fmt.Sprintf(".%s", strings.ToLower(realm))] = realm

	printKrb5Conf(krb5Conf)

	// Connect to LDAP server
	bindString := fmt.Sprintf("ldaps://%s:636", fqdnLDAPHost)
	ldapConnection, err := ldap.DialURL(
		bindString,
		ldap.DialWithTLSConfig(
			&tls.Config{
				InsecureSkipVerify: true,
			},
		),
	)
	if err != nil {
		log.Printf("[error] ldap.DialURL(\"%s\"): %s\n", bindString, err)
		return
	} else {
		log.Printf("[debug] ldap.DialURL(\"%s\"): success\n", bindString)
	}
	ldapConnection.Debug = true

	// Initialize kerberos client
	// Inspired from: https://github.com/go-ldap/ldap/blob/06d50d1ad03bcd323e48f2fe174d95ceb31b8b90/v3/gssapi/client.go#L51
	kerberosClient := gssapi.Client{
		Client: client.NewWithPassword(
			username,
			realm,
			password,
			krb5Conf,
			// Active Directory does not commonly support FAST negotiationso you will need to disable this on the client.
			// If this is the case you will see this error: KDC did not respond appropriately to FAST negotiation
			// https://github.com/jcmturner/gokrb5/blob/master/USAGE.md#active-directory-kdc-and-fast-negotiation
			client.DisablePAFXFAST(true),
		),
	}
	defer kerberosClient.Close()

	// Initiating ldap GSSAPIBind
	err = ldapConnection.GSSAPIBindRequest(
		&kerberosClient,
		&ldap.GSSAPIBindRequest{
			ServicePrincipalName: servicePrincipalName,
			AuthZID:              "",
		},
		[]int{flags.APOptionMutualRequired},
        )
	if err != nil {
		log.Printf("[error] ldapConnection.GSSAPIBind(): %s\n", err)
		return
	} else {
		log.Printf("[debug] ldapConnection.GSSAPIBind(): success\n")
	}

	// Successfully bound
	searchRequest := ldap.NewSearchRequest(
		baseDN,
		ldap.ScopeWholeSubtree,
		ldap.NeverDerefAliases,
		0,
		0,
		false,
		"(objectClass=user)",
		[]string{"distinguishedName"},
		nil,
	)
	ldapResults, err := ldapConnection.SearchWithPaging(searchRequest, 1000)
	if err != nil {
		log.Fatalf("[error] ldapConnection.Search(): %v\n", err)
		return
	} else {
		log.Printf("[debug] ldapConnection.Search(): success\n")
	}

	for _, entry := range ldapResults.Entries {
		fmt.Printf(" - %s", entry.DN)
	}

	log.Printf("[debug] All done!\n")
}
}

Summary

These APOptions []int can now be set when calling client.GSSAPIBindRequest(...), which will then pass it to the underlying client.InitSecContext(...), which will then be processed in the call to spnego.NewKRB5TokenAPREQ(client.Client, tkt, ekey, gssapiFlags, APOptions)

Best Regards,

…tSecContext(...), fixes go-ldap#536

cpuschma · 2024-11-15T19:32:49Z

Hi @p0dalirius,

thank you very much for your PR and the work you put into debugging this issue (#536)! My strengths are not at all in Kerberos and my knowledge of the protocol is practically zero, so thank you again!

But: Modifying the signature of an existing function will definitely interfere with existing code. Even the addition of changes that are only noticed by linters has caused trouble in the past (#463).

If there is really no other way to implement Kerberos authentication, I would be willing to do so and include it in the next release, but only if no one objects. @go-ldap/committers thoughts?

…id changing the func prototypes

p0dalirius · 2024-11-18T12:09:26Z

@cpuschma, before merging, It would be nice if someone can try to connect to a UNIX ldap server with GSSAPI to confirm that it still works with this fix

I will try to setup one and test it

stefanmcshane · 2024-11-20T21:39:29Z

Hi @p0dalirius,

thank you very much for your PR and the work you put into debugging this issue (#536)! My strengths are not at all in Kerberos and my knowledge of the protocol is practically zero, so thank you again!

But: Modifying the signature of an existing function will definitely interfere with existing code. Even the addition of changes that are only noticed by linters has caused trouble in the past (#463).

If there is really no other way to implement Kerberos authentication, I would be willing to do so and include it in the next release, but only if no one objects. @go-ldap/committers thoughts?

Using functional options instead of a slice of ints should not break existing implementations

stevensbkang · 2024-12-11T21:39:43Z

Could someone please advise if this will be merged soon? :)

johnweldon

One of the difficulties with this PR is the lack of GSSAPI knowledge by many of the maintainers - if someone with GSSAPI experience could weigh in that would be helpful.

That being said there are some things that should be done differently, and I've tried to point out some of them in this review.

I like the idea of using functional options to avoid future breaking changes, but I'm not sure there's really a need for it in this particular case.

v3/gssapi/client.go

johnweldon · 2024-12-13T02:11:21Z

v3/gssapi/client.go

@@ -99,7 +103,7 @@ func (client *Client) DeleteSecContext() error {
 // InitSecContext initiates the establishment of a security context for
 // GSS-API between the client and server.
 // See RFC 4752 section 3.1.
-func (client *Client) InitSecContext(target string, input []byte) ([]byte, bool, error) {
+func (client *Client) InitSecContext(target string, input []byte, APOptions []int) ([]byte, bool, error) {


We should not break the API.

I suggest adding a new func (*Client) InitSecContextWithOptions(string, []byte, []int) ([]byte, bool, error) and then modify the internals of the original InitSecContext to simply call the new InitSecContextWithOptions, passing in suitable defaults.

johnweldon · 2024-12-13T02:13:33Z

v3/gssapi/client.go

@@ -212,3 +216,48 @@ func (client *Client) NegotiateSaslAuth(input []byte, authzid string) ([]byte, e

 	return output, nil
 }
+
+func UnmarshalWrapToken(wt *gssapi.WrapToken, data []byte, expectFromAcceptor bool) error {


I appreciate this new function that appears to do more robust checking while unmarshalling the input, however:

We should not export the function (and thereby add to our API), unless absolutely necessary.

I would be much more comfortable if there were some unit tests asserting that this function actually works as intended.

johnweldon · 2024-12-13T02:16:30Z

v3/gssapi/client.go

+func UnmarshalWrapToken(wt *gssapi.WrapToken, data []byte, expectFromAcceptor bool) error {
+	// Check if we can read a whole header
+	if len(data) < 16 {
+		return errors.New("bytes shorter than header length")


If we anticipate any kind of code handling of these errors it may be kinder to actually create named errors (var ErrInvalidGSSAPIFooBarHeaderLength = "bytes shorter ..."), that can be used with the errors.Is and related helper functions.

johnweldon · 2024-12-13T02:18:33Z

v3/gssapi/client.go

+	// Is the Token ID correct?
+	expectedWrapTokenId := [2]byte{0x05, 0x04}
+	if !bytes.Equal(expectedWrapTokenId[:], data[0:2]) {
+		return fmt.Errorf("wrong Token ID. Expected %s, was %s", hex.EncodeToString(expectedWrapTokenId[:]), hex.EncodeToString(data[0:2]))


In cases where returning additional context with the error it may be nice for it to wrap a simpler 'known' error as the related comment says.

eg return fmt.Errorf("wrong blah blah. expected %s, was %s: %w", x, y, ErrUnmarshalFault) or something like that

johnweldon · 2024-12-13T02:20:41Z

v3/bind.go

@@ -576,7 +576,7 @@ type GSSAPIClient interface {
 	// reply token is received from the server, passing the reply token
 	// to InitSecContext via the token parameters.
 	// See RFC 4752 section 3.1.
-	InitSecContext(target string, token []byte) (outputToken []byte, needContinue bool, err error)
+	InitSecContext(target string, token []byte, APOptions []int) (outputToken []byte, needContinue bool, err error)


As mentioned in the comment on the implementation; let's not modify (break) an existing API - we should add a new method if we really need to.

…ient struct

FlipB · 2024-12-20T11:58:59Z

The earlier commits would have broken the client in gssapi/sspi.go.
These last changes keep the krb5 specific details in the krb5 client, so that's good.

(I think it makes sense to add asserts that both clients implement the interface to make breaking changes less likely, although you would still have to build for Windows (looks like CI only builds for linux) to be sure.)

p0dalirius force-pushed the fix-ldap-gssapi-kerberos-auth-on-windows-active-directory branch from c76a77b to 3a20a5c Compare November 14, 2024 15:14

Added APOptions []int in client.GSSAPIBindRequest(...) and client.Ini…

fe5ed31

…tSecContext(...), fixes go-ldap#536

p0dalirius force-pushed the fix-ldap-gssapi-kerberos-auth-on-windows-active-directory branch from 3a20a5c to fe5ed31 Compare November 14, 2024 15:17

p0dalirius mentioned this pull request Nov 14, 2024

[bug] Could not make a working code example on connecting to LDAP with Kerberos from a linux machine to a Windows Server #536

Open

cpuschma added the enhancement label Nov 15, 2024

cpuschma self-assigned this Nov 15, 2024

cpuschma added bug and removed enhancement labels Nov 15, 2024

Moving APOptions to GSSAPIBindRequest struct for coherence and to avo…

3bbd598

…id changing the func prototypes

Unified UnmarshalWrapToken() with v3 and base

78ef5d4

johnweldon requested changes Dec 13, 2024

View reviewed changes

p0dalirius added 4 commits December 13, 2024 09:00

Update client.go

1cad944

Removed breaking API changes to InitSecContext, added APOptions in Cl…

1cc1eb3

…ient struct

Removed breaking API changes to InitSecContext, added APOptions in Cl…

f752bf8

…ient struct

Changing unmarshalWrapToken function to private

a55dbd6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix] Added APOptions []int in client.GSSAPIBindRequest(...) and client.InitSecContext(...), fixes #536 #537

[bugfix] Added APOptions []int in client.GSSAPIBindRequest(...) and client.InitSecContext(...), fixes #536 #537

p0dalirius commented Nov 14, 2024 •

edited

Loading

cpuschma commented Nov 15, 2024

p0dalirius commented Nov 18, 2024 •

edited

Loading

stefanmcshane commented Nov 20, 2024

stevensbkang commented Dec 11, 2024

johnweldon left a comment

johnweldon Dec 13, 2024

johnweldon Dec 13, 2024

johnweldon Dec 13, 2024

johnweldon Dec 13, 2024

johnweldon Dec 13, 2024

FlipB commented Dec 20, 2024

[bugfix] Added APOptions []int in client.GSSAPIBindRequest(...) and client.InitSecContext(...), fixes #536 #537

Are you sure you want to change the base?

[bugfix] Added APOptions []int in client.GSSAPIBindRequest(...) and client.InitSecContext(...), fixes #536 #537

Conversation

p0dalirius commented Nov 14, 2024 • edited Loading

Fixing Kerberos authentication due to missing APOption "MutualRequired"

Overview of the fix

In client.InitSecContext(...)

In client.GSSAPIBind(...)

In client.GSSAPIBindRequest(...)

Example of a working code for Kerberos authentication

Summary

cpuschma commented Nov 15, 2024

p0dalirius commented Nov 18, 2024 • edited Loading

stefanmcshane commented Nov 20, 2024

stevensbkang commented Dec 11, 2024

johnweldon left a comment

Choose a reason for hiding this comment

johnweldon Dec 13, 2024

Choose a reason for hiding this comment

johnweldon Dec 13, 2024

Choose a reason for hiding this comment

johnweldon Dec 13, 2024

Choose a reason for hiding this comment

johnweldon Dec 13, 2024

Choose a reason for hiding this comment

johnweldon Dec 13, 2024

Choose a reason for hiding this comment

FlipB commented Dec 20, 2024

p0dalirius commented Nov 14, 2024 •

edited

Loading

In `client.InitSecContext(...)`

In `client.GSSAPIBind(...)`

In `client.GSSAPIBindRequest(...)`

p0dalirius commented Nov 18, 2024 •

edited

Loading