diff --git a/builtin/credential/approle/path_login.go b/builtin/credential/approle/path_login.go index 65a9ec4f57..a550c719a4 100644 --- a/builtin/credential/approle/path_login.go +++ b/builtin/credential/approle/path_login.go @@ -33,7 +33,11 @@ func pathLogin(b *backend) *framework.Path { // Returns the Auth object indicating the authentication and authorization information // if the credentials provided are validated by the backend. func (b *backend) pathLoginUpdate(req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - role, roleName, metadata, err := b.validateCredentials(req, data) + if req.Connection == nil || req.Connection.RemoteAddr == "" { + return nil, fmt.Errorf("failed to get connection information") + } + + role, roleName, metadata, err := b.validateCredentials(req, data, req.Connection.RemoteAddr) if err != nil || role == nil { return logical.ErrorResponse(fmt.Sprintf("failed to validate SecretID: %s", err)), nil } diff --git a/builtin/credential/approle/path_login_test.go b/builtin/credential/approle/path_login_test.go index 03d3f68c34..211b40fbcb 100644 --- a/builtin/credential/approle/path_login_test.go +++ b/builtin/credential/approle/path_login_test.go @@ -43,6 +43,9 @@ func TestAppRole_RoleLogin(t *testing.T) { Path: "login", Storage: storage, Data: loginData, + Connection: &logical.Connection{ + RemoteAddr: "127.0.0.1", + }, } resp, err = b.HandleRequest(loginReq) if err != nil || (resp != nil && resp.IsError()) { diff --git a/builtin/credential/approle/path_role.go b/builtin/credential/approle/path_role.go index 683c307014..563c574ebe 100644 --- a/builtin/credential/approle/path_role.go +++ b/builtin/credential/approle/path_role.go @@ -1738,39 +1738,17 @@ func (b *backend) handleRoleSecretIDCommon(req *logical.Request, data *framework // Parse the CIDR blocks into a slice secretIDCIDRs := strutil.ParseDedupAndSortStrings(cidrList, ",") - if len(secretIDCIDRs) != 0 { - // If role has bound_cidr_list set, check if each CIDR block is - // a subset of at least one CIDR block registered under the - // role. If bound_cidr_list is not set on the role, then it - // means that the role allows any IP address, implying that the - // CIDR blocks on the secret ID are a subset of that of role's. - roleCIDRs := strutil.ParseDedupAndSortStrings(role.BoundCIDRList, ",") - if len(roleCIDRs) != 0 { - subset, err := cidrutil.SubsetBlocks(roleCIDRs, secretIDCIDRs) - if err != nil { - return nil, fmt.Errorf("failed to verify subset relationship between CIDR blocks on the role %q and CIDR blocks on the secret ID %q: %v", roleCIDRs, secretIDCIDRs, err) - } - if !subset { - return logical.ErrorResponse(fmt.Sprintf("CIDR blocks on the secret ID %q should be a subset of CIDR blocks on the role %q", secretIDCIDRs, roleCIDRs)), nil - } - } - if req.Connection == nil || req.Connection.RemoteAddr == "" { - return nil, fmt.Errorf("failed to get connection information") - } - belongs, err := cidrutil.IPBelongsToCIDRBlocksSlice(req.Connection.RemoteAddr, secretIDCIDRs) - if err != nil { - return nil, fmt.Errorf("failed to check if IP belonged to configured CIDR blocks on the secret ID") - } - if !belongs { - return logical.ErrorResponse(fmt.Sprintf("unauthorized source address")), nil - } + // Ensure that the CIDRs on the secret ID are a subset of that of role's + if err := verifyCIDRRoleSecretIDSubset(secretIDCIDRs, role.BoundCIDRList); err != nil { + return nil, err } secretIDStorage := &secretIDStorageEntry{ SecretIDNumUses: role.SecretIDNumUses, SecretIDTTL: role.SecretIDTTL, Metadata: make(map[string]string), + CIDRList: secretIDCIDRs, } if err = strutil.ParseArbitraryKeyValues(data.Get("metadata").(string), secretIDStorage.Metadata, ","); err != nil { diff --git a/builtin/credential/approle/path_role_test.go b/builtin/credential/approle/path_role_test.go index 4f6521002a..217c9f7349 100644 --- a/builtin/credential/approle/path_role_test.go +++ b/builtin/credential/approle/path_role_test.go @@ -45,11 +45,11 @@ func TestAppRole_CIDRSubset(t *testing.T) { } resp, err = b.HandleRequest(secretIDReq) - if err != nil { - t.Fatal(err) + if resp != nil || resp.IsError() { + t.Fatalf("resp:%#v", resp) } - if resp == nil || !resp.IsError() { - t.Fatalf("resp:%#v", err, resp) + if err == nil { + t.Fatal("expected an error") } roleData["bound_cidr_list"] = "192.168.27.29/16,172.245.30.40/24,10.20.30.40/30" diff --git a/builtin/credential/approle/validation.go b/builtin/credential/approle/validation.go index b936e77d49..93742efb67 100644 --- a/builtin/credential/approle/validation.go +++ b/builtin/credential/approle/validation.go @@ -5,12 +5,13 @@ import ( "crypto/sha256" "encoding/hex" "fmt" - "net" "strings" "sync" "time" "github.com/hashicorp/go-uuid" + "github.com/hashicorp/vault/helper/cidrutil" + "github.com/hashicorp/vault/helper/strutil" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" ) @@ -26,32 +27,38 @@ type secretIDStorageEntry struct { // SecretIDs during login. SecretIDAccessor string `json:"secret_id_accessor" structs:"secret_id_accessor" mapstructure:"secret_id_accessor"` - // Number of times this SecretID can be used to perform the login operation - SecretIDNumUses int `json:"secret_id_num_uses" structs:"secret_id_num_uses" mapstructure:"secret_id_num_uses"` + // Number of times this SecretID can be used to perform the login + // operation + SecretIDNumUses int `json:"secret_id_num_uses" +structs:"secret_id_num_uses" mapstructure:"secret_id_num_uses"` - // Duration after which this SecretID should expire. This is - // croleed by the backend mount's max TTL value. + // Duration after which this SecretID should expire. This is croleed by + // the backend mount's max TTL value. SecretIDTTL time.Duration `json:"secret_id_ttl" structs:"secret_id_ttl" mapstructure:"secret_id_ttl"` // The time when the SecretID was created CreationTime time.Time `json:"creation_time" structs:"creation_time" mapstructure:"creation_time"` - // The time when the SecretID becomes eligible for tidy - // operation. Tidying is performed by the PeriodicFunc of the - // backend which is 1 minute apart. + // The time when the SecretID becomes eligible for tidy operation. + // Tidying is performed by the PeriodicFunc of the backend which is 1 + // minute apart. ExpirationTime time.Time `json:"expiration_time" structs:"expiration_time" mapstructure:"expiration_time"` // The time representing the last time this storage entry was modified LastUpdatedTime time.Time `json:"last_updated_time" structs:"last_updated_time" mapstructure:"last_updated_time"` - // Metadata that belongs to the SecretID. + // Metadata that belongs to the SecretID Metadata map[string]string `json:"metadata" structs:"metadata" mapstructure:"metadata"` + + // CIDRList is a set of CIDR blocks that impose source address + // restrictions on the usage of SecretID + CIDRList []string `json:"cidr_list" structs:"cidr_list" mapstructure:"cidr_list"` } -// Represents the payload of the storage entry of the accessor that maps to a unique -// SecretID. Note that SecretIDs should never be stored in plaintext anywhere in the -// backend. SecretIDHMAC will be used as an index to fetch the properties of the -// SecretID and to delete the SecretID. +// Represents the payload of the storage entry of the accessor that maps to a +// unique SecretID. Note that SecretIDs should never be stored in plaintext +// anywhere in the backend. SecretIDHMAC will be used as an index to fetch the +// properties of the SecretID and to delete the SecretID. type secretIDAccessorStorageEntry struct { // Hash of the SecretID which can be used to find the storage index at which // properties of SecretID is stored. @@ -81,7 +88,7 @@ func (b *backend) validateRoleID(s logical.Storage, roleID string) (*roleStorage } // Validates the supplied RoleID and SecretID -func (b *backend) validateCredentials(req *logical.Request, data *framework.FieldData) (*roleStorageEntry, string, map[string]string, error) { +func (b *backend) validateCredentials(req *logical.Request, data *framework.FieldData, sourceIP string) (*roleStorageEntry, string, map[string]string, error) { var metadata map[string]string // RoleID must be supplied during every login roleID := strings.TrimSpace(data.Get("role_id").(string)) @@ -114,7 +121,7 @@ func (b *backend) validateCredentials(req *logical.Request, data *framework.Fiel // Check if the SecretID supplied is valid. If use limit was specified // on the SecretID, it will be decremented in this call. var valid bool - valid, metadata, err = b.validateBindSecretID(req.Storage, roleName, secretID, role.HMACKey) + valid, metadata, err = b.validateBindSecretID(req.Storage, roleName, secretID, role.HMACKey, role.BoundCIDRList, sourceIP) if err != nil { return nil, "", metadata, err } @@ -125,20 +132,12 @@ func (b *backend) validateCredentials(req *logical.Request, data *framework.Fiel if role.BoundCIDRList != "" { // If 'bound_cidr_list' was set, verify the CIDR restrictions - cidrBlocks := strings.Split(role.BoundCIDRList, ",") - for _, block := range cidrBlocks { - _, cidr, err := net.ParseCIDR(block) - if err != nil { - return nil, "", metadata, fmt.Errorf("invalid cidr: %s", err) - } - - var addr string - if req.Connection != nil { - addr = req.Connection.RemoteAddr - } - if addr == "" || !cidr.Contains(net.ParseIP(addr)) { - return nil, "", metadata, fmt.Errorf("unauthorized source address") - } + belongs, err := cidrutil.IPBelongsToCIDRBlocksString(sourceIP, role.BoundCIDRList, ",") + if err != nil { + return nil, "", metadata, fmt.Errorf("failed to verify the CIDR restrictions set on the role: %v", err) + } + if !belongs { + return nil, "", metadata, fmt.Errorf("source address unauthorized through CIDR restrictions on the role") } } @@ -146,7 +145,8 @@ func (b *backend) validateCredentials(req *logical.Request, data *framework.Fiel } // validateBindSecretID is used to determine if the given SecretID is a valid one. -func (b *backend) validateBindSecretID(s logical.Storage, roleName, secretID, hmacKey string) (bool, map[string]string, error) { +func (b *backend) validateBindSecretID(s logical.Storage, roleName, secretID, + hmacKey, roleBoundCIDRList, sourceIP string) (bool, map[string]string, error) { secretIDHMAC, err := createHMAC(hmacKey, secretID) if err != nil { return false, nil, fmt.Errorf("failed to create HMAC of secret_id: %s", err) @@ -181,6 +181,21 @@ func (b *backend) validateBindSecretID(s logical.Storage, roleName, secretID, hm // in which case, the SecretID will remain to be valid as long as it is not // expired. if result.SecretIDNumUses == 0 { + // Ensure that the CIDRs on the secret ID are still a subset of that of + // role's + if err := verifyCIDRRoleSecretIDSubset(result.CIDRList, + roleBoundCIDRList); err != nil { + return false, nil, err + } + + // If CIDR restrictions are present on the secret ID, check if the + // source IP complies to it + if len(result.CIDRList) != 0 { + if belongs, err := cidrutil.IPBelongsToCIDRBlocksSlice(sourceIP, result.CIDRList); !belongs || err != nil { + return false, nil, fmt.Errorf("source address unauthorized through CIDR restrictions on the secret ID: %v", err) + } + } + lock.RUnlock() return true, result.Metadata, nil } @@ -225,9 +240,44 @@ func (b *backend) validateBindSecretID(s logical.Storage, roleName, secretID, hm } } + // Ensure that the CIDRs on the secret ID are still a subset of that of + // role's + if err := verifyCIDRRoleSecretIDSubset(result.CIDRList, + roleBoundCIDRList); err != nil { + return false, nil, err + } + + // If CIDR restrictions are present on the secret ID, check if the + // source IP complies to it + if len(result.CIDRList) != 0 { + if belongs, err := cidrutil.IPBelongsToCIDRBlocksSlice(sourceIP, result.CIDRList); !belongs || err != nil { + return false, nil, fmt.Errorf("source address unauthorized through CIDR restrictions on the secret ID: %v", err) + } + } + return true, result.Metadata, nil } +// verifyCIDRRoleSecretIDSubset checks if the CIDR blocks set on the secret ID +// are a subset of CIDR blocks set on the role +func verifyCIDRRoleSecretIDSubset(secretIDCIDRs []string, roleBoundCIDRList string) error { + if len(secretIDCIDRs) != 0 { + // Parse the CIDRs on role as a slice + roleCIDRs := strutil.ParseDedupAndSortStrings(roleBoundCIDRList, ",") + + // If there are no CIDR blocks on the role, then the subset + // requirement would be satisfied + if len(roleCIDRs) != 0 { + subset, err := cidrutil.SubsetBlocks(roleCIDRs, secretIDCIDRs) + if !subset || err != nil { + return fmt.Errorf("failed to verify subset relationship between CIDR blocks on the role %q and CIDR blocks on the secret ID %q: %v", roleCIDRs, secretIDCIDRs, err) + } + } + } + + return nil +} + // Creates a SHA256 HMAC of the given 'value' using the given 'key' and returns // a hex encoded string. func createHMAC(key, value string) (string, error) { diff --git a/helper/cidrutil/cidr.go b/helper/cidrutil/cidr.go index 338a08a542..7b3dfbcf10 100644 --- a/helper/cidrutil/cidr.go +++ b/helper/cidrutil/cidr.go @@ -124,23 +124,23 @@ func Subset(cidr1, cidr2 string) (bool, error) { return false, fmt.Errorf("missing CIDR that needs to be checked") } - _, net1, err := net.ParseCIDR(cidr1) + ip1, net1, err := net.ParseCIDR(cidr1) if err != nil { return false, fmt.Errorf("failed to parse the CIDR to be checked against: %q", err) } maskLen1, _ := net1.Mask.Size() - if maskLen1 == 0 { + if ip1.To4().String() != "0.0.0.0" && maskLen1 == 0 { return false, fmt.Errorf("CIDR to be checked against is not in its canonical form") } - _, net2, err := net.ParseCIDR(cidr2) + ip2, net2, err := net.ParseCIDR(cidr2) if err != nil { return false, fmt.Errorf("failed to parse the CIDR that needs to be checked: %q", err) } maskLen2, _ := net2.Mask.Size() - if maskLen2 == 0 { + if ip2.To4().String() != "0.0.0.0" && maskLen2 == 0 { return false, fmt.Errorf("CIDR that needs to be checked is not in its canonical form") } @@ -179,7 +179,7 @@ func SubsetBlocks(cidrBlocks1, cidrBlocks2 []string) (bool, error) { isSubset := false for _, cidrBlock1 := range cidrBlocks1 { subset, err := Subset(cidrBlock1, cidrBlock2) - if !subset && err != nil { + if err != nil { return false, err } // If CIDR is a subset of any of the CIDR block, its diff --git a/website/source/docs/auth/approle.html.md b/website/source/docs/auth/approle.html.md index f337c6cbb9..31b966e44d 100644 --- a/website/source/docs/auth/approle.html.md +++ b/website/source/docs/auth/approle.html.md @@ -479,6 +479,16 @@ $ curl -XPOST "http://127.0.0.1:8200/v1/auth/approle/login" -d '{"role_id":"50be _in plaintext_. +
Returns
@@ -721,6 +731,16 @@ $ curl -XPOST "http://127.0.0.1:8200/v1/auth/approle/login" -d '{"role_id":"50be _in plaintext_. +
Returns