Implement SASL reader #170
Conversation
|
I think the CI failure is caused by a race condition. |
|
Hi @dtaniwaki, This looks great, but I'm a bit leery of merging it without any tests. Could you please add some? I'd add another whole branch here, to run the test suite with those settings enabled. |
|
@colinmarc I added RPC protection environment matrix. Is it what you meant? |
Makefile
Outdated
| @@ -9,6 +9,7 @@ PROTO_MAPPING = MSecurity.proto=github.com/colinmarc/hdfs/v2/internal/protocol/h | |||
| TRAVIS_TAG ?= $(shell git rev-parse HEAD) | |||
| ARCH = $(shell go env GOOS)-$(shell go env GOARCH) | |||
| RELEASE_NAME = gohdfs-$(TRAVIS_TAG)-$(ARCH) | |||
| _CLIENT = $(shell kubectl get pods -l app=hdfs-client,release=my-hdfs -o name | cut -d/ -f 2) | |||
There was a problem hiding this comment.
I think this snuck in from your local testing environment?
There was a problem hiding this comment.
Oops! Thank you for pointing it out!
internal/rpc/challenge.go
Outdated
|
|
||
| const ( | ||
| // Authentication - Establishes mutual authentication between the client and the server | ||
| Authentication = QualityOfProtection("auth") |
There was a problem hiding this comment.
Just make these string constants, I think.
internal/rpc/challenge.go
Outdated
| tokenChallenge := TokenChallenge{} | ||
| matched := challengeRegexp.FindAllSubmatch(auth.Challenge, -1) | ||
| if matched == nil { | ||
| return nil, errors.New("aaaa") |
There was a problem hiding this comment.
You need to pass the error up here (with a more informative message ;))
There was a problem hiding this comment.
I don’t know what happened with this error message :P I’ll fix it.
internal/rpc/kerberos.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Read the final response. If it's a SUCCESS, then we're done here. | ||
| _, err = c.readSaslResponse(hadoop.RpcSaslProto_SUCCESS) | ||
| if err != nil { |
internal/rpc/kerberos.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| if challenge.QOP == Privacy || challenge.QOP == Integrity { |
There was a problem hiding this comment.
We should error on a QOP we can't handle.
internal/rpc/rpc_handler.go
Outdated
| // To check interface validity | ||
| var rpcWriter RpcWriter = &BasicRpcWriter{} | ||
| var rpcReader RpcReader = &BasicRpcReader{} | ||
| var saslRpcReader RpcReader = &SaslRpcReader{} |
There was a problem hiding this comment.
This isn't necessary - the interface validity is checked elsewhere in the code.
| var saslRpcReader RpcReader = &SaslRpcReader{} | ||
|
|
||
| // RpcWriter is an interface for sending RPC payload | ||
| type RpcWriter interface { |
There was a problem hiding this comment.
I think this is basically, right, but can you call it 'Transport'? There's a good chance the Transport should track the request sequence number, too.
There was a problem hiding this comment.
Could you tell me what is supposed to call Transport in detail?
I guess your comment above is more than just renaming the RpcWriter interface.
internal/rpc/rpc_handler.go
Outdated
| type BasicRpcReader struct { | ||
| } | ||
|
|
||
| // SaslRpcReader is a RPC reader which wrap payload with SASL |
There was a problem hiding this comment.
Sorry for being picky, but can you change all these comments to be complete sentences?
|
@colinmarc I updated the PR based on your review comment. Would you check it again? I will squash the commits after review if necessary. |
|
Hi Best! |
|
I am using kerberos and also hit the unexpected sequence number issue so could use this. Any ETA? |
|
@colinmarc @dtaniwaki - Are there any plans for merging this? The partial workaround provided in issue #144 worked so I know I need this functionality to get things fully working. Thanks |
|
Merged in 1c841f7. Thanks for your help, and sorry for the delay! |
I made RPC writing and reading interface and implemented SASL reader which is supposed to be used when a GSS API server responded with
TOKENauth and the QOP isauth-conforauth-int.I tried all the following patterns of qop config in
core-site.xmland all of them work fine with this change.hadoop.rpc.protection:authenticationhadoop.rpc.protection:integrityhadoop.rpc.protection:privacyI didn't implement
SaslRpcWriteras it requires a client to choose QOP and a lot of changes around the command line tool is necessary. I will start to implement it once this PR gets merged.Solves: #144