Negotiate improvements #4

vanackere · 2018-05-22T14:48:47Z

Hi,

This branch contains various changes / improvements to the negotiate package (FYI those changes were added in order to implement both Web SPNEGO and SASL GSS-SPNEGO support, using the native windows api).

Please merge if you feel those are appropriate, thanks !

vanackere · 2018-05-22T14:50:09Z

Note: I had to change the prototype from sspi.AcquireCredentials in order to pass the principal name since the C++ code I was porting made use of this parameter. Not breaking the current signature would require a new function (sspi.AcquireCredentialsForPrincipal ?), please let me know if you feel this is a better option.

alexbrainman · 2018-05-25T23:41:43Z

I will try to review this weekend.

Alex

alexbrainman

Most of it looks fine to me.

Thank you.

Alex

alexbrainman · 2018-05-28T00:33:49Z

negotiate/negotiate.go

 	return makeSignature(c.sctxt, msg, qop, seqno)
 }

+// EncryptMessage uses the established client context to encrypt a message


Can you, please, add a test for EncryptMessage and DecryptMessage ?

alexbrainman · 2018-05-28T00:39:35Z

negotiate/negotiate.go

-// AcquireUserCredentials and SPN to start client Negotiate
+// AcquireUserCredentials and SPN to start a client Negotiate
 // negotiation sequence. targetName is the service principal name
 // (SPN) or the security context of the destination server.


You do not mention new "flags" parameter in the documentation. Don't you want to say something about it?

You did not address my comment.

The flags parameter is documented in AcquireUserCredentialsWithFlags, did you mean something else ?

My fault, I have no idea why I asked you to document "flag" parameter here. Forget my comment.

alexbrainman · 2018-05-28T00:44:54Z

sspi.go

 }

-func AcquireCredentials(pkgname string, creduse uint32, authdata *byte) (*Credentials, error) {
+func AcquireCredentials(principal string, pkgname string, creduse uint32, authdata *byte) (*Credentials, error) {


This commit changes the way "principal" parameter of "" is handled. What is happening here is not obvious. This needs to be at least documented in function doco.

An empty principal parameter is passed as a nil pointer in the underlying winapi call. I will document.

I still do not see any new documentation that you promised.

I forgot this part, sorry. I'll fix that.

Please, do.

alexbrainman · 2018-05-28T00:47:19Z

negotiate/negotiate.go

 }

 // AcquireServerCredentials acquires server credentials that will
 // be used to authenticate client.


Maybe add new "principalName" parameter documentation.

You did not address my comment.

I'm sorry, I'm afraid I did not understand what you meant here... Do you suggest to add a new parameter "principalName" to AcquireUserCredentials ?

You have added "principalName" parameter to AcquireServerCredentials function. You should document new parameter - explain what it does.

alexbrainman · 2018-05-28T00:54:52Z

negotiate/negotiate.go

+	if ret != sspi.SEC_E_OK {
+		return "", ret
+	}
+	defer sspi.FreeContextBuffer((*byte)(unsafe.Pointer(ns.ClientName)))


Why aren't you freeing ServerName too?

It was simply an oversight, fixed !

vanackere · 2018-05-29T14:34:11Z

Hi, I made the requested changes and updated my branch, PTAL

alexbrainman

I still see some of my comments not addressed. Please, let me know, if I am missing something.

Alex

alexbrainman · 2018-05-31T09:17:47Z

negotiate/negotiate.go

 }

 // AcquireServerCredentials acquires server credentials that will
 // be used to authenticate client.


You did not address my comment.

alexbrainman · 2018-05-31T09:18:21Z

sspi.go

 }

-func AcquireCredentials(pkgname string, creduse uint32, authdata *byte) (*Credentials, error) {
+func AcquireCredentials(principal string, pkgname string, creduse uint32, authdata *byte) (*Credentials, error) {


I still do not see any new documentation that you promised.

alexbrainman · 2018-05-31T09:18:49Z

negotiate/negotiate.go

-// AcquireUserCredentials and SPN to start client Negotiate
+// AcquireUserCredentials and SPN to start a client Negotiate
 // negotiation sequence. targetName is the service principal name
 // (SPN) or the security context of the destination server.


You did not address my comment.

alexbrainman

I hope that I explained myself better this time.

And, please, rebase your changes, because I just pushed new change into this repo.

Thank you.

Alex

alexbrainman · 2018-06-01T02:30:18Z

negotiate/negotiate.go

-// AcquireUserCredentials and SPN to start client Negotiate
+// AcquireUserCredentials and SPN to start a client Negotiate
 // negotiation sequence. targetName is the service principal name
 // (SPN) or the security context of the destination server.


My fault, I have no idea why I asked you to document "flag" parameter here. Forget my comment.

alexbrainman · 2018-06-01T02:33:18Z

negotiate/negotiate.go

 }

 // AcquireServerCredentials acquires server credentials that will
 // be used to authenticate client.


You have added "principalName" parameter to AcquireServerCredentials function. You should document new parameter - explain what it does.

alexbrainman · 2018-06-01T02:33:49Z

sspi.go

 }

-func AcquireCredentials(pkgname string, creduse uint32, authdata *byte) (*Credentials, error) {
+func AcquireCredentials(principal string, pkgname string, creduse uint32, authdata *byte) (*Credentials, error) {


Please, do.

This allows to pass specific useful flags (like sspi.ISC_REQ_INTEGRITY or ISC_REQ_INTEGRITY)

Setting this parameter to an empty string will pass nil to the underlying windows sspi function. Only the negotiate public api is changed in this commit (since the C++ code I'm porting was explicitely using this parameter).

This method returns the username corresponding to the authenticated client

Explicitely returning the serverDone boolean allows for slightly cleaner code

vanackere · 2018-06-01T09:57:19Z

Branch updated and rebased, please let me know if I still missed anything, thanks !

alexbrainman · 2018-06-02T01:37:18Z

LGTM now. Thank you.

Alex

alexbrainman · 2018-06-02T01:38:35Z

Merged, so closing this now.

Alex

alexbrainman requested changes May 28, 2018

View reviewed changes

vanackere force-pushed the negotiate-improvements branch from 48f578e to 816b5ac Compare May 29, 2018 14:30

alexbrainman requested changes May 31, 2018

View reviewed changes

alexbrainman requested changes Jun 1, 2018

View reviewed changes

vanackere added 5 commits June 1, 2018 11:37

negotiate: add EncryptMessage / DecryptMessage

cf85fdd

negotiate: add NewClientContextWithFlags

ce601ab

This allows to pass specific useful flags (like sspi.ISC_REQ_INTEGRITY or ISC_REQ_INTEGRITY)

sspi: add principal name parameter to AcquireCredentials

0af8175

Setting this parameter to an empty string will pass nil to the underlying windows sspi function. Only the negotiate public api is changed in this commit (since the C++ code I'm porting was explicitely using this parameter).

negotiate: add GetUsername to ServerContext

57a3c9d

This method returns the username corresponding to the authenticated client

negotiate: minor NewServerContext cleanup

55a2fc7

Explicitely returning the serverDone boolean allows for slightly cleaner code

vanackere force-pushed the negotiate-improvements branch from 816b5ac to 55a2fc7 Compare June 1, 2018 09:38

sspi: add comments to AcquireCredentials

033374a

enj mentioned this pull request Jun 1, 2018

Implement SSPI Support on Windows (oc Kerberos) openshift/origin#11371

Merged

6 tasks

alexbrainman merged commit 033374a into alexbrainman:master Jun 2, 2018

Negotiate improvements #4

Negotiate improvements #4

Uh oh!

Conversation

vanackere commented May 22, 2018

Uh oh!

vanackere commented May 22, 2018

Uh oh!

alexbrainman commented May 25, 2018

Uh oh!

alexbrainman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanackere commented May 29, 2018

Uh oh!

alexbrainman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexbrainman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanackere commented Jun 1, 2018

Uh oh!

alexbrainman commented Jun 2, 2018

Uh oh!

alexbrainman commented Jun 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!