[wip][do not review] auth support for dbnode#4235
[wip][do not review] auth support for dbnode#4235gandhikrishna wants to merge 18 commits intom3db:masterfrom
Conversation
SokolAndrey
left a comment
SokolAndrey
left a comment
There was a problem hiding this comment.
This is very big PR, please consider splitting it into smaller parts
| AutoSyncInterval time.Duration `yaml:"autoSyncInterval"` | ||
| DialTimeout time.Duration `yaml:"dialTimeout"` | ||
|
|
||
| Auth *AuthConfig `yaml:"auth"` |
There was a problem hiding this comment.
pls don't use pointers in config structures
|
|
||
| // Validate validates the AuthConfig. | ||
| func (a *AuthConfig) Validate() error { | ||
| if a.Enabled { |
There was a problem hiding this comment.
nit:
if !a.Enabled { return nil } ...
| func (a *AuthConfig) Validate() error { | ||
| if a.Enabled { | ||
| if a.UserName == "" || a.Password == "" { | ||
| return fmt.Errorf("credentials must be set for client's outbound") |
There was a problem hiding this comment.
super nit: avoid using single quote in the log messages
| // AuthConfig is the top level config that includes secrets of inbound and | ||
| // outbound of a dbnode. | ||
| type AuthConfig struct { | ||
| Outbound *Outbounds `yaml:"outbounds"` |
There was a problem hiding this comment.
same as above, avoid using pointers in config structs. Once you replace it with values I don't think you need all these validation methods
| services: | ||
| - service: | ||
| zone: m3_abc | ||
| username: m3_bcd |
There was a problem hiding this comment.
I thought this should be a path to a file, no?
| return nil | ||
| } | ||
|
|
||
| if i.authMode == AuthModeShadow { |
There was a problem hiding this comment.
this should be after cred validation
| } | ||
|
|
||
| if i.authMode == AuthModeShadow { | ||
| go func() { |
There was a problem hiding this comment.
Why is it executed in a separated routine?
| }() | ||
| return nil | ||
| } | ||
| if creds.Type == Unknown { |
There was a problem hiding this comment.
I think this should be part of Validate
| // ValidateCredentialsFromThriftContext validates inbound credential from thrift context. | ||
| func (i *Inbound) ValidateCredentialsFromThriftContext(tctx thrift.Context, credtype CredentialType) error { | ||
| ctxHeaders := tctx.Headers() | ||
| userName, ok := ctxHeaders[AuthUsername] |
There was a problem hiding this comment.
I think we discussed it offline, but why don't we encrypt the username/password before sending it as a header? We provide user name/password to the node at start up, we might as well pass a private part of asymetric key. User encrypt the username/password using public part before sending the request and we decrypt it here.
I don't think it will add a lot of overhead but without it sending username/password in plain text as a header doesn't provide a lot of security
|
|
||
| var ( | ||
| credentialCache = map[string]string{} | ||
| hashFunc = md5.New() // #nosec |
There was a problem hiding this comment.
maybe I'm missing something but are we not using nonce? Using digest authentication is vulnerable to simple replay attack
2 participants
What this PR does / why we need it:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: