From aaadd4ad97ae44877727b7a61188625fc7dd8780 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Wed, 21 Sep 2016 19:41:08 -0400 Subject: [PATCH] Store the CIDR list in the secret ID storage entry. Use the stored information to validate the source address and credential issue time. Correct the logic used to verify BoundCIDRList on the role. Reverify the subset requirements between secret ID and role during credential issue time. --- builtin/credential/approle/path_login.go | 6 +- builtin/credential/approle/path_login_test.go | 3 + builtin/credential/approle/path_role.go | 30 +---- builtin/credential/approle/path_role_test.go | 8 +- builtin/credential/approle/validation.go | 110 +++++++++++++----- helper/cidrutil/cidr.go | 10 +- website/source/docs/auth/approle.html.md | 20 ++++ 7 files changed, 121 insertions(+), 66 deletions(-) diff --git a/builtin/credential/approle/path_login.go b/builtin/credential/approle/path_login.go index 65a9ec4f5..a550c719a 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 03d3f68c3..211b40fbc 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 683c30701..563c574eb 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 4f6521002..217c9f734 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 b936e77d4..93742efb6 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 338a08a54..7b3dfbcf1 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 f337c6cbb..31b966e44 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