Login MFA #14025
Login MFA #14025
Conversation
raskchanky
force-pushed
the
vault-4554-oss
branch
from
636e957
to
874c12d
Compare
vault/login_mfa.go
Outdated
| return false, nil | ||
| } | ||
|
|
||
| func (b *SystemBackend) mfaPaths() []*framework.Path { |
There was a problem hiding this comment.
I think this might need to be on ENT side only
| } | ||
| } | ||
|
|
||
| func (c *Core) PersistTOTPKey(ctx context.Context, methodID, entityID, key string) error { |
There was a problem hiding this comment.
This is redefined in identity_store_oss.go as well.
vault/login_mfa.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func mfaConfigTableSchema() *memdb.TableSchema { |
There was a problem hiding this comment.
Maybe this also belongs to ENT?
vault/login_mfa.go
Outdated
| if namespaceId == namespace.RootNamespaceID { | ||
| barrierView = b.Core.systemBarrierView | ||
| } else { | ||
| barrierView = b.Core.nsView.SubView(path.Join(namespaceId, systemBarrierPrefix) + "/") |
There was a problem hiding this comment.
in OSS, there seems to be no b.Core.nsView. So, maybe barrierViewForNamespace would need to have different functionality in OSS and ENT?
| // that NS or its children | ||
| //- an entity may configure TOTP keys for methods defined in the entity's NS or | ||
| // its parents | ||
| func TestNamespaceLoginMfaTotp(t *testing.T) { |
There was a problem hiding this comment.
This test belong to ENT side
| Core: core, | ||
| db: db, | ||
| logger: logger, | ||
| mfaBackend: NewPolicyMFABackend(core, logger), |
There was a problem hiding this comment.
NewPolicyMFABackend needs to be defined in OSS as well. Currently, this is only defined in ENT in policy_mfa_ent.go
Also, not sure if NewSystemBackend should even configure mfaBackend this way. Maybe, configuring the mfa part should be done in an ENT specific module?
| Core *Core | ||
| db *memdb.MemDB | ||
| logger log.Logger | ||
| mfaBackend *PolicyMFABackend |
There was a problem hiding this comment.
Should this be something generic like MFABackend?
There was a problem hiding this comment.
@ncabatoff did the split between PolicyMFABackend and LoginMFABackend, but I think this is correct to stay as it is, because the existing enterprise MFA functionality that uses the system backend is policy MFA.
There was a problem hiding this comment.
Well in OSS, we don't support PolicyMFABackend. Keeping this as is would not resolve in OSS as PolicyMFABackend is defined policy_mfa_ent.go right now.
There was a problem hiding this comment.
Ah yes, of course.
There was a problem hiding this comment.
We could probably setup something similar to how the Core struct hides enterprise specific fields. https://github.com/hashicorp/vault-enterprise/blob/main/vault/core.go#L199
|
It seems that we would need to run |
| // login MFA. The following paths are supported: | ||
| // mfa/method-id/:mfa_method - management of MFA method IDs, which can be used for configuration | ||
| // mfa/login_enforcement/:config_name - configures single or two phase MFA auth | ||
| func (b *SystemBackend) loginMFAPaths() []*framework.Path { |
There was a problem hiding this comment.
This is not hooked right now as far as I could tell. Should it be added here?
Lines 163 to 187 in 23301d7
vault/request_handling.go
Outdated
| RequestNSPath: ns.Path, | ||
| RequestConnRemoteAddr: req.Connection.RemoteAddr, // this is needed for the DUO method | ||
| TimeOfStorage: time.Now(), | ||
| RequestID: req.ID, |
There was a problem hiding this comment.
Shouldn't this be mfaRequestID?
* Delete an MFA methodID only if it is not used by an MFA enforcement config * Fixing a bug: mfa/validate is an unauthenticated path, and goes through the handleLoginRequest path
* preventing replay attack on MFA passcodes * using %w instead of %s for error
CLI prints a warning message indicating the login request needs to get validated
command/write.go
Outdated
| @@ -146,6 +146,11 @@ func (c *WriteCommand) Run(args []string) int { | |||
| return 0 | |||
| } | |||
|
|
|||
| if secret != nil && secret.Auth != nil && secret.Auth.MFARequirement != nil { | |||
| c.UI.Warn(wrapAtLength("WARNING! A login request was issued that is subject to "+ | |||
There was a problem hiding this comment.
I think all-caps "WARNING" may be a little too strong. I'd leave it as a UI.Warn but drop the "WARNING" since it's really more informative - like if someone is doing this routinely I think it makes this a bit more alarming than it needs to be.
vault/core_util.go
Outdated
|
|
||
| func (c *Core) barrierViewForNamespace(namespaceId string) (*BarrierView, error) { | ||
| if namespaceId != namespace.RootNamespaceID { | ||
| return nil, fmt.Errorf("faild to find barrier view for non-root namespace") |
There was a problem hiding this comment.
| return nil, fmt.Errorf("faild to find barrier view for non-root namespace") | |
| return nil, fmt.Errorf("failed to find barrier view for non-root namespace") |
vault/request_handling.go
Outdated
| // going to return early before generating the token | ||
| // the user receives the mfaRequirement, and need to use the | ||
| // login MFA validate endpoint to get the token | ||
| return resp, auth, retErr |
There was a problem hiding this comment.
| return resp, auth, retErr | |
| return resp, auth, nil |
retErr should only be non-nil if an error occurred earlier, in which case we'd have already returned.
| @@ -0,0 +1,3 @@ | |||
| ```release-note:feature | |||
There was a problem hiding this comment.
@raskchanky Catching this in final changelog review, but as a feature this should have the "new feature" formatting, which is on the changelog page on the wiki.
There was a problem hiding this comment.
Argh, sorry 😞 . I can submit a PR to change it.
OSS version of https://github.com/hashicorp/vault-enterprise/pull/2433