-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Login MFA #14025
Login MFA #14025
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test belong to ENT side
Core: core, | ||
db: db, | ||
logger: logger, | ||
mfaBackend: NewPolicyMFABackend(core, logger), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be something generic like MFABackend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not hooked right now as far as I could tell. Should it be added here?
Lines 163 to 187 in 23301d7
b.Backend.Paths = append(b.Backend.Paths, entPaths(b)...) | |
b.Backend.Paths = append(b.Backend.Paths, b.configPaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.rekeyPaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.sealPaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.statusPaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.pluginsCatalogListPaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.pluginsCatalogCRUDPath()) | |
b.Backend.Paths = append(b.Backend.Paths, b.pluginsReloadPath()) | |
b.Backend.Paths = append(b.Backend.Paths, b.auditPaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.mountPaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.authPaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.leasePaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.policyPaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.wrappingPaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.toolsPaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.capabilitiesPaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.internalPaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.pprofPaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.remountPath()) | |
b.Backend.Paths = append(b.Backend.Paths, b.metricsPath()) | |
b.Backend.Paths = append(b.Backend.Paths, b.monitorPath()) | |
b.Backend.Paths = append(b.Backend.Paths, b.inFlightRequestPath()) | |
b.Backend.Paths = append(b.Backend.Paths, b.hostInfoPath()) | |
b.Backend.Paths = append(b.Backend.Paths, b.quotasPaths()...) | |
b.Backend.Paths = append(b.Backend.Paths, b.rootActivityPaths()...) |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, nice work!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh, sorry 😞 . I can submit a PR to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OSS version of https://github.com/hashicorp/vault-enterprise/pull/2433