Skip to content

Commit d53acc6

Browse files
JabejixoJabejixo
committed
apply suggestions from review
Signed-off-by: Ivan Zvyagintsev <ivan.zvyagintsev@flant.com>
1 parent 264cc60 commit d53acc6

2 files changed

Lines changed: 90 additions & 113 deletions

File tree

‎connector/ldap/kerberos.go‎

Lines changed: 78 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -42,28 +42,35 @@ func writeNegotiateChallenge(w http.ResponseWriter) {
4242
}
4343

4444
// mapPrincipal maps a Kerberos principal to LDAP username per configuration.
45-
func mapPrincipal(principal, realm, mapping string) string {
46-
p := principal
45+
// Supported mappings:
46+
// - "localpart" / "samaccountname": extracts username before @ (default)
47+
// - "userprincipalname": uses full principal as-is
48+
func mapPrincipal(principal, mapping string) string {
49+
// Extract localpart (before @) from principal
50+
localpart := principal
51+
if i := strings.IndexByte(principal, '@'); i >= 0 {
52+
localpart = principal[:i]
53+
}
54+
4755
switch strings.ToLower(mapping) {
48-
case "localpart", "samaccountname":
49-
if i := strings.IndexByte(principal, '@'); i >= 0 {
50-
p = principal[:i]
51-
}
52-
return strings.ToLower(p)
5356
case "userprincipalname":
5457
return strings.ToLower(principal)
58+
case "localpart", "samaccountname":
59+
return strings.ToLower(localpart)
5560
default:
56-
if i := strings.IndexByte(principal, '@'); i >= 0 {
57-
p = principal[:i]
58-
}
59-
return strings.ToLower(p)
61+
return strings.ToLower(localpart)
6062
}
6163
}
6264

6365
// gokrb5 implementation of KerberosValidator
6466

65-
// context key used by gokrb5 to store credentials in the context
66-
var ctxCredentialsKey interface{} = "github.com/jcmturner/gokrb5/v8/ctxCredentials"
67+
// ctxCredentialsKeyType is the type for gokrb5 context key.
68+
// gokrb5 uses a string constant as context key for storing credentials.
69+
type ctxCredentialsKeyType string
70+
71+
// ctxCredentialsKey is the context key used by gokrb5 to store credentials.
72+
// This must match the exact string used in gokrb5/v8/spnego package.
73+
const ctxCredentialsKey ctxCredentialsKeyType = "github.com/jcmturner/gokrb5/v8/ctxCredentials"
6774

6875
// SPNEGO NegTokenResp (AcceptIncomplete + KRB5 mech) base64 payload used by gokrb5's HTTP server
6976
// to prompt the client to continue the handshake.
@@ -75,69 +82,82 @@ type gokrb5Validator struct {
7582
}
7683

7784
func newGokrb5ValidatorWithLogger(keytabPath string, logger *slog.Logger) (KerberosValidator, error) {
85+
fi, err := os.Stat(keytabPath)
86+
if err != nil {
87+
return nil, fmt.Errorf("keytab file not found: %w", err)
88+
}
89+
if fi.IsDir() {
90+
return nil, fmt.Errorf("keytab path is a directory: %s", keytabPath)
91+
}
7892
kt, err := keytab.Load(keytabPath)
7993
if err != nil {
8094
return nil, fmt.Errorf("failed to load keytab: %w", err)
8195
}
82-
if fi, err := os.Stat(keytabPath); err != nil || fi.IsDir() {
83-
return nil, fmt.Errorf("invalid keytab path: %s", keytabPath)
84-
}
8596
if logger == nil {
8697
logger = slog.Default()
8798
}
8899
return &gokrb5Validator{kt: kt, logger: logger}, nil
89100
}
90101

91-
func (v *gokrb5Validator) ValidateRequest(r *http.Request) (string, string, bool, error) {
102+
// parseNegotiateHeader extracts and decodes the SPNEGO token from Authorization header.
103+
// Returns (token, ok). If ok is false, the header is missing, malformed, or not Negotiate.
104+
func (v *gokrb5Validator) parseNegotiateHeader(r *http.Request) (*spnego.SPNEGOToken, bool) {
92105
h := r.Header.Get("Authorization")
93106
if h == "" || !strings.HasPrefix(h, "Negotiate ") {
94-
if v.logger != nil {
95-
v.logger.Info("kerberos: missing or non-negotiate Authorization header", "path", r.URL.Path)
96-
}
97-
return "", "", false, nil
107+
return nil, false
98108
}
99109
b64 := strings.TrimSpace(h[len("Negotiate "):])
100110
if b64 == "" {
101-
if v.logger != nil {
102-
v.logger.Info("kerberos: empty negotiate token", "path", r.URL.Path)
103-
}
104-
return "", "", false, nil
111+
return nil, false
105112
}
106113
data, err := base64.StdEncoding.DecodeString(b64)
107114
if err != nil {
108115
if v.logger != nil {
109116
v.logger.Info("kerberos: invalid base64 in Authorization", "err", err)
110117
}
111-
return "", "", false, nil
118+
return nil, false
112119
}
113120
var tok spnego.SPNEGOToken
114121
if err := tok.Unmarshal(data); err != nil {
115-
// Try raw KRB5 token and wrap
122+
// Try raw KRB5 token and wrap it as SPNEGO
116123
var k5 spnego.KRB5Token
117124
if k5.Unmarshal(data) != nil {
118125
if v.logger != nil {
119-
v.logger.Info("kerberos: failed to unmarshal SPNEGO token and not raw KRB5", "err", err)
126+
v.logger.Info("kerberos: failed to unmarshal SPNEGO/KRB5 token", "err", err)
120127
}
121-
return "", "", false, nil
128+
return nil, false
122129
}
123130
tok.Init = true
124131
tok.NegTokenInit = spnego.NegTokenInit{
125132
MechTypes: []asn1.ObjectIdentifier{k5.OID},
126133
MechTokenBytes: data,
127134
}
128135
}
136+
return &tok, true
137+
}
129138

130-
// Pass client address when available (improves AP-REQ validation with address-bound tickets)
131-
var sp *spnego.SPNEGO
139+
// createSPNEGOService creates a SPNEGO service with optional client address binding.
140+
func (v *gokrb5Validator) createSPNEGOService(r *http.Request) *spnego.SPNEGO {
132141
if ha, err := types.GetHostAddress(r.RemoteAddr); err == nil {
133-
sp = spnego.SPNEGOService(v.kt, service.ClientAddress(ha), service.DecodePAC(false))
134-
} else {
142+
return spnego.SPNEGOService(v.kt, service.ClientAddress(ha), service.DecodePAC(false))
143+
}
144+
if v.logger != nil {
145+
v.logger.Info("kerberos: cannot parse client address", "remote", r.RemoteAddr)
146+
}
147+
return spnego.SPNEGOService(v.kt, service.DecodePAC(false))
148+
}
149+
150+
func (v *gokrb5Validator) ValidateRequest(r *http.Request) (string, string, bool, error) {
151+
tok, ok := v.parseNegotiateHeader(r)
152+
if !ok {
135153
if v.logger != nil {
136-
v.logger.Info("kerberos: cannot parse client address", "remote", r.RemoteAddr, "err", err)
154+
v.logger.Info("kerberos: missing or invalid Negotiate header", "path", r.URL.Path)
137155
}
138-
sp = spnego.SPNEGOService(v.kt, service.DecodePAC(false))
156+
return "", "", false, nil
139157
}
140-
authed, ctx, status := sp.AcceptSecContext(&tok)
158+
159+
sp := v.createSPNEGOService(r)
160+
authed, ctx, status := sp.AcceptSecContext(tok)
141161
if status.Code != gssapi.StatusComplete {
142162
if v.logger != nil {
143163
v.logger.Info("kerberos: AcceptSecContext not complete", "code", status.Code, "message", status.Message)
@@ -165,74 +185,37 @@ func (v *gokrb5Validator) Challenge(w http.ResponseWriter) { writeNegotiateChall
165185
// ContinueToken attempts to continue the SPNEGO handshake and returns a response token
166186
// (to be placed into WWW-Authenticate: Negotiate <b64>) if available.
167187
func (v *gokrb5Validator) ContinueToken(r *http.Request) ([]byte, bool) {
168-
h := r.Header.Get("Authorization")
169-
if h == "" || !strings.HasPrefix(h, "Negotiate ") {
170-
if v.logger != nil {
171-
v.logger.Info("kerberos: ContinueToken without negotiate header", "path", r.URL.Path)
172-
}
173-
return nil, false
174-
}
175-
b64 := strings.TrimSpace(h[len("Negotiate "):])
176-
data, err := base64.StdEncoding.DecodeString(b64)
177-
if err != nil {
178-
// Malformed header: ask client to continue with KRB5 mech
179-
if tok, e := base64.StdEncoding.DecodeString(spnegoIncompleteKRB5B64); e == nil {
180-
if v.logger != nil {
181-
v.logger.Info("kerberos: malformed negotiate token; sending incomplete KRB5 response")
182-
}
183-
return tok, true
184-
}
185-
return nil, false
186-
}
187-
var tok spnego.SPNEGOToken
188-
if err := tok.Unmarshal(data); err != nil {
189-
// Not a full SPNEGO token; still ask client to continue
190-
if tokb, e := base64.StdEncoding.DecodeString(spnegoIncompleteKRB5B64); e == nil {
191-
if v.logger != nil {
192-
v.logger.Info("kerberos: non-SPNEGO token; sending incomplete KRB5 response")
193-
}
194-
return tokb, true
195-
}
196-
// As a fallback, try wrapping as raw KRB5
197-
var k5 spnego.KRB5Token
198-
if k5.Unmarshal(data) != nil {
199-
if v.logger != nil {
200-
v.logger.Info("kerberos: not KRB5 token; cannot continue")
201-
}
202-
return nil, false
203-
}
204-
tok.Init = true
205-
tok.NegTokenInit = spnego.NegTokenInit{MechTypes: []asn1.ObjectIdentifier{k5.OID}, MechTokenBytes: data}
206-
}
207-
// Try continue with same options as in ValidateRequest
208-
var sp *spnego.SPNEGO
209-
if ha, err := types.GetHostAddress(r.RemoteAddr); err == nil {
210-
sp = spnego.SPNEGOService(v.kt, service.ClientAddress(ha), service.DecodePAC(false))
211-
} else {
212-
sp = spnego.SPNEGOService(v.kt, service.DecodePAC(false))
188+
tok, ok := v.parseNegotiateHeader(r)
189+
if !ok {
190+
// No valid token; return incomplete response to prompt client
191+
return v.incompleteResponse()
213192
}
214-
_, ctx, status := sp.AcceptSecContext(&tok)
193+
194+
sp := v.createSPNEGOService(r)
195+
_, ctx, status := sp.AcceptSecContext(tok)
215196
if status.Code != gssapi.StatusContinueNeeded || ctx == nil {
216197
if v.logger != nil {
217198
v.logger.Info("kerberos: no continuation required", "code", status.Code, "message", status.Message)
218199
}
219200
return nil, false
220201
}
221-
// Ask client to continue using standard NegTokenResp (KRB5, incomplete)
222-
if tokb, e := base64.StdEncoding.DecodeString(spnegoIncompleteKRB5B64); e == nil {
223-
if v.logger != nil {
224-
v.logger.Info("kerberos: continuation needed; sending incomplete KRB5 response")
225-
}
226-
return tokb, true
202+
return v.incompleteResponse()
203+
}
204+
205+
// incompleteResponse returns the standard NegTokenResp to prompt client continuation.
206+
func (v *gokrb5Validator) incompleteResponse() ([]byte, bool) {
207+
tokb, err := base64.StdEncoding.DecodeString(spnegoIncompleteKRB5B64)
208+
if err != nil {
209+
return nil, false
227210
}
228-
return nil, false
211+
if v.logger != nil {
212+
v.logger.Info("kerberos: sending incomplete KRB5 response for continuation")
213+
}
214+
return tokb, true
229215
}
230216

231217
// LDAP connector SPNEGO integration
232218

233-
// krbLookupUserHook allows tests to inject a user entry without LDAP queries.
234-
var krbLookupUserHook func(c *ldapConnector, username string) (ldap.Entry, bool, error)
235-
236219
// TrySPNEGO attempts Kerberos auth and builds identity on success.
237220
func (c *ldapConnector) TrySPNEGO(ctx context.Context, s connector.Scopes, w http.ResponseWriter, r *http.Request) (*connector.Identity, connector.Handled, error) {
238221
if !c.krbEnabled || c.krbValidator == nil {
@@ -271,13 +254,13 @@ func (c *ldapConnector) TrySPNEGO(ctx context.Context, s connector.Scopes, w htt
271254
return nil, false, nil
272255
}
273256

274-
mapped := mapPrincipal(principal, realm, c.krbConf.UsernameFromPrincipal)
257+
mapped := mapPrincipal(principal, c.krbConf.UsernameFromPrincipal)
275258
c.logger.Info("kerberos principal mapped", "principal", principal, "realm", realm, "mapped_username", mapped)
276259

277260
var userEntry ldap.Entry
278261
// Allow test hook override
279-
if krbLookupUserHook != nil {
280-
if v, found, herr := krbLookupUserHook(c, mapped); found {
262+
if c.krbLookupUserHook != nil {
263+
if v, found, herr := c.krbLookupUserHook(c, mapped); found {
281264
if herr != nil {
282265
return nil, true, herr
283266
}

‎connector/ldap/kerberos_test.go‎

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -169,15 +169,14 @@ func TestKerberos_ContinueThenSuccess_ShortCircuitIdentity(t *testing.T) {
169169
}
170170

171171
// Second request -> validator now returns ok=true (due to step increment)
172-
krbLookupUserHook = func(c *ldapConnector, username string) (ldaplib.Entry, bool, error) {
172+
lc.krbLookupUserHook = func(c *ldapConnector, username string) (ldaplib.Entry, bool, error) {
173173
e := ldaplib.NewEntry("cn=jdoe,dc=example,dc=org", map[string][]string{
174174
c.UserSearch.IDAttr: {"uid-jdoe"},
175175
c.UserSearch.EmailAttr: {"jdoe@example.com"},
176176
c.UserSearch.NameAttr: {"John Doe"},
177177
})
178178
return *e, true, nil
179179
}
180-
defer func() { krbLookupUserHook = nil }()
181180
r2 := httptest.NewRequest("GET", "/auth/ldap/login?state=abc", nil)
182181
w2 := httptest.NewRecorder()
183182
ident2, handled2, err2 := lc.TrySPNEGO(r2.Context(), connector.Scopes{}, w2, r2)
@@ -208,15 +207,15 @@ func TestKerberos_ContinueNeeded_FallbackTrue_NotHandled(t *testing.T) {
208207
}
209208

210209
func TestKerberos_mapPrincipal(t *testing.T) {
211-
cases := []struct{ in, realm, mode, want string }{
212-
{"JDoe@EXAMPLE.COM", "EXAMPLE.COM", "localpart", "jdoe"},
213-
{"JDoe@EXAMPLE.COM", "EXAMPLE.COM", "sAMAccountName", "jdoe"},
214-
{"JDoe@EXAMPLE.COM", "EXAMPLE.COM", "userPrincipalName", "jdoe@example.com"},
210+
cases := []struct{ in, mode, want string }{
211+
{"JDoe@EXAMPLE.COM", "localpart", "jdoe"},
212+
{"JDoe@EXAMPLE.COM", "sAMAccountName", "jdoe"},
213+
{"JDoe@EXAMPLE.COM", "userPrincipalName", "jdoe@example.com"},
215214
}
216215
for _, c := range cases {
217-
got := mapPrincipal(c.in, c.realm, c.mode)
216+
got := mapPrincipal(c.in, c.mode)
218217
if got != c.want {
219-
t.Fatalf("mapPrincipal(%q,%q,%q)=%q; want %q", c.in, c.realm, c.mode, got, c.want)
218+
t.Fatalf("mapPrincipal(%q,%q)=%q; want %q", c.in, c.mode, got, c.want)
220219
}
221220
}
222221
}
@@ -249,15 +248,14 @@ func TestKerberos_ValidPrincipal_CompletesFlow(t *testing.T) {
249248
lc.Config.UserSearch.NameAttr = "cn"
250249
mv := &mockKrbValidator{principal: "jdoe@EXAMPLE.COM", realm: "EXAMPLE.COM", ok: true, err: nil, step: -1}
251250
lc.krbValidator = mv
252-
krbLookupUserHook = func(c *ldapConnector, username string) (ldaplib.Entry, bool, error) {
251+
lc.krbLookupUserHook = func(c *ldapConnector, username string) (ldaplib.Entry, bool, error) {
253252
e := ldaplib.NewEntry("cn=jdoe,dc=example,dc=org", map[string][]string{
254253
c.UserSearch.IDAttr: {"uid-jdoe"},
255254
c.UserSearch.EmailAttr: {"jdoe@example.com"},
256255
c.UserSearch.NameAttr: {"John Doe"},
257256
})
258257
return *e, true, nil
259258
}
260-
defer func() { krbLookupUserHook = nil }()
261259
r := httptest.NewRequest("GET", "/auth/ldap/login?state=abc", nil)
262260
w := httptest.NewRecorder()
263261
ident, handled, err := lc.TrySPNEGO(r.Context(), connector.Scopes{}, w, r)
@@ -307,7 +305,7 @@ func TestKerberos_UserPrincipalName_Mapping(t *testing.T) {
307305
lc.Config.UserSearch.NameAttr = "cn"
308306
mv := &mockKrbValidator{principal: "J.Doe@Example.COM", realm: "Example.COM", ok: true, err: nil, step: -1}
309307
lc.krbValidator = mv
310-
krbLookupUserHook = func(c *ldapConnector, username string) (ldaplib.Entry, bool, error) {
308+
lc.krbLookupUserHook = func(c *ldapConnector, username string) (ldaplib.Entry, bool, error) {
311309
if username != "j.doe@example.com" {
312310
return ldaplib.Entry{}, false, nil
313311
}
@@ -318,7 +316,6 @@ func TestKerberos_UserPrincipalName_Mapping(t *testing.T) {
318316
})
319317
return *e, true, nil
320318
}
321-
defer func() { krbLookupUserHook = nil }()
322319
r := httptest.NewRequest("GET", "/auth/ldap/login?state=abc", nil)
323320
w := httptest.NewRecorder()
324321
ident, handled, err := lc.TrySPNEGO(r.Context(), connector.Scopes{}, w, r)
@@ -340,15 +337,14 @@ func TestKerberos_OfflineAccess_SetsConnectorData(t *testing.T) {
340337
lc.Config.UserSearch.NameAttr = "cn"
341338
mv := &mockKrbValidator{principal: "jdoe@EXAMPLE.COM", realm: "EXAMPLE.COM", ok: true, err: nil, step: -1}
342339
lc.krbValidator = mv
343-
krbLookupUserHook = func(c *ldapConnector, username string) (ldaplib.Entry, bool, error) {
340+
lc.krbLookupUserHook = func(c *ldapConnector, username string) (ldaplib.Entry, bool, error) {
344341
e := ldaplib.NewEntry("cn=jdoe,dc=example,dc=org", map[string][]string{
345342
c.UserSearch.IDAttr: {"uid-jdoe"},
346343
c.UserSearch.EmailAttr: {"jdoe@example.com"},
347344
c.UserSearch.NameAttr: {"John Doe"},
348345
})
349346
return *e, true, nil
350347
}
351-
defer func() { krbLookupUserHook = nil }()
352348
r := httptest.NewRequest("GET", "/auth/ldap/login?state=abc", nil)
353349
w := httptest.NewRecorder()
354350
scopes := connector.Scopes{OfflineAccess: true}
@@ -396,7 +392,7 @@ func TestKerberos_sAMAccountName_EqualsLocalpart(t *testing.T) {
396392
lc.Config.UserSearch.NameAttr = "cn"
397393
mv := &mockKrbValidator{principal: "Admin@REALM.LOCAL", realm: "REALM.LOCAL", ok: true, err: nil, step: -1}
398394
lc.krbValidator = mv
399-
krbLookupUserHook = func(c *ldapConnector, username string) (ldaplib.Entry, bool, error) {
395+
lc.krbLookupUserHook = func(c *ldapConnector, username string) (ldaplib.Entry, bool, error) {
400396
if username != "admin" {
401397
return ldaplib.Entry{}, false, nil
402398
}
@@ -407,7 +403,6 @@ func TestKerberos_sAMAccountName_EqualsLocalpart(t *testing.T) {
407403
})
408404
return *e, true, nil
409405
}
410-
defer func() { krbLookupUserHook = nil }()
411406
r := httptest.NewRequest("GET", "/auth/ldap/login?state=abc", nil)
412407
w := httptest.NewRecorder()
413408
ident, handled, err := lc.TrySPNEGO(r.Context(), connector.Scopes{}, w, r)
@@ -429,15 +424,14 @@ func TestKerberos_ExpectedRealm_CaseInsensitive(t *testing.T) {
429424
lc.Config.UserSearch.NameAttr = "cn"
430425
mv := &mockKrbValidator{principal: "user@EXAMPLE.COM", realm: "EXAMPLE.COM", ok: true, err: nil, step: -1}
431426
lc.krbValidator = mv
432-
krbLookupUserHook = func(c *ldapConnector, username string) (ldaplib.Entry, bool, error) {
427+
lc.krbLookupUserHook = func(c *ldapConnector, username string) (ldaplib.Entry, bool, error) {
433428
e := ldaplib.NewEntry("cn=user,dc=example,dc=com", map[string][]string{
434429
c.UserSearch.IDAttr: {"uid-user"},
435430
c.UserSearch.EmailAttr: {"user@example.com"},
436431
c.UserSearch.NameAttr: {"User"},
437432
})
438433
return *e, true, nil
439434
}
440-
defer func() { krbLookupUserHook = nil }()
441435
r := httptest.NewRequest("GET", "/auth/ldap/login?state=abc", nil)
442436
w := httptest.NewRecorder()
443437
ident, handled, err := lc.TrySPNEGO(r.Context(), connector.Scopes{}, w, r)

0 commit comments

Comments
 (0)