Skip to content
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

Restrict cert auth by CIDR #4478

Merged
merged 13 commits into from
May 9, 2018
126 changes: 126 additions & 0 deletions builtin/credential/cert/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1033,6 +1033,132 @@ func TestBackend_untrusted(t *testing.T) {
})
}

func TestBackend_validCIDR(t *testing.T) {
config := logical.TestBackendConfig()
storage := &logical.InmemStorage{}
config.StorageView = storage

b, err := Factory(context.Background(), config)
if err != nil {
t.Fatal(err)
}

connState, err := testConnState("test-fixtures/keys/cert.pem",
"test-fixtures/keys/key.pem", "test-fixtures/root/rootcacert.pem")
if err != nil {
t.Fatalf("error testing connection state: %v", err)
}
ca, err := ioutil.ReadFile("test-fixtures/root/rootcacert.pem")
if err != nil {
t.Fatalf("err: %v", err)
}

name := "web"

addCertReq := &logical.Request{
Operation: logical.UpdateOperation,
Path: "certs/" + name,
Data: map[string]interface{}{
"certificate": string(ca),
"policies": "foo",
"display_name": name,
"allowed_names": "",
"required_extensions": "",
"lease": 1000,
"bound_cidrs": []string{"127.0.0.1/32", "128.252.0.0/16"},
},
Storage: storage,
Connection: &logical.Connection{ConnState: &connState},
}

_, err = b.HandleRequest(context.Background(), addCertReq)
if err != nil {
t.Fatal(err)
}

loginReq := &logical.Request{
Operation: logical.UpdateOperation,
Path: "login",
Unauthenticated: true,
Data: map[string]interface{}{
"name": name,
},
Storage: storage,
Connection: &logical.Connection{ConnState: &connState},
}

// override the remote address with an IPV4 that is authorized
loginReq.Connection.RemoteAddr = "127.0.0.1/32"

_, err = b.HandleRequest(context.Background(), loginReq)
if err != nil {
t.Fatal(err.Error())
}
}

func TestBackend_invalidCIDR(t *testing.T) {
config := logical.TestBackendConfig()
storage := &logical.InmemStorage{}
config.StorageView = storage

b, err := Factory(context.Background(), config)
if err != nil {
t.Fatal(err)
}

connState, err := testConnState("test-fixtures/keys/cert.pem",
"test-fixtures/keys/key.pem", "test-fixtures/root/rootcacert.pem")
if err != nil {
t.Fatalf("error testing connection state: %v", err)
}
ca, err := ioutil.ReadFile("test-fixtures/root/rootcacert.pem")
if err != nil {
t.Fatalf("err: %v", err)
}

name := "web"

addCertReq := &logical.Request{
Operation: logical.UpdateOperation,
Path: "certs/" + name,
Data: map[string]interface{}{
"certificate": string(ca),
"policies": "foo",
"display_name": name,
"allowed_names": "",
"required_extensions": "",
"lease": 1000,
"bound_cidrs": []string{"127.0.0.1/32", "128.252.0.0/16"},
},
Storage: storage,
Connection: &logical.Connection{ConnState: &connState},
}

_, err = b.HandleRequest(context.Background(), addCertReq)
if err != nil {
t.Fatal(err)
}

loginReq := &logical.Request{
Operation: logical.UpdateOperation,
Path: "login",
Unauthenticated: true,
Data: map[string]interface{}{
"name": name,
},
Storage: storage,
Connection: &logical.Connection{ConnState: &connState},
}

// override the remote address with an IPV4 that isn't authorized
loginReq.Connection.RemoteAddr = "127.0.0.1/8"

_, err = b.HandleRequest(context.Background(), loginReq)
if err == nil {
t.Fatal("expected \"ERROR: permission denied\"")
}
}

func testAccStepAddCRL(t *testing.T, crl []byte, connState tls.ConnectionState) logicaltest.TestStep {
return logicaltest.TestStep{
Operation: logical.UpdateOperation,
Expand Down
25 changes: 25 additions & 0 deletions builtin/credential/cert/path_certs.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"
"time"

"github.com/hashicorp/go-sockaddr"
"github.com/hashicorp/vault/helper/policyutil"
"github.com/hashicorp/vault/logical"
"github.com/hashicorp/vault/logical/framework"
Expand Down Expand Up @@ -88,6 +89,11 @@ should never expire. The token should be renewed within the
duration specified by this value. At each renewal, the token's
TTL will be set to the value of this parameter.`,
},
"bound_cidrs": &framework.FieldSchema{
Type: framework.TypeCommaStringSlice,
Description: `Comma separated string or list of CIDR blocks. If set, specifies the blocks of
IP addresses which can perform the login operation.`,
},
},

Callbacks: map[logical.Operation]framework.OperationFunc{
Expand Down Expand Up @@ -228,6 +234,23 @@ func (b *backend) pathCertWrite(ctx context.Context, req *logical.Request, d *fr
}
}

var parsedCIDRs []*sockaddr.SockAddrMarshaler
if boundCIDRListRaw, ok := d.GetOk("bound_cidrs"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern is usually only required for paths that handle both create and update operations (have an existence check). I think you could simplify this by just iterating of the the list without checking if the parameter is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


var boundCIDRList []string
if boundCIDRs, ok := boundCIDRListRaw.([]string); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a zero value for TypeCommaStringSlice of nil slice and this cast checking is not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌮

boundCIDRList = boundCIDRs
}

for _, v := range boundCIDRList {
parsedCIDR, err := sockaddr.NewSockAddr(v)
if err != nil {
return nil, err
}
parsedCIDRs = append(parsedCIDRs, &sockaddr.SockAddrMarshaler{parsedCIDR})
}
}

certEntry := &CertEntry{
Name: name,
Certificate: certificate,
Expand All @@ -238,6 +261,7 @@ func (b *backend) pathCertWrite(ctx context.Context, req *logical.Request, d *fr
TTL: ttl,
MaxTTL: maxTTL,
Period: period,
BoundCIDRs: parsedCIDRs,
}

// Store it
Expand Down Expand Up @@ -266,6 +290,7 @@ type CertEntry struct {
Period time.Duration
AllowedNames []string
RequiredExtensions []string
BoundCIDRs []*sockaddr.SockAddrMarshaler
}

const pathCertHelpSyn = `
Expand Down
38 changes: 38 additions & 0 deletions builtin/credential/cert/path_login.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/hashicorp/vault/logical"
"github.com/hashicorp/vault/logical/framework"

"github.com/hashicorp/go-sockaddr"
"github.com/ryanuber/go-glob"
)

Expand Down Expand Up @@ -71,6 +72,10 @@ func (b *backend) pathLogin(ctx context.Context, req *logical.Request, data *fra
return nil, nil
}

if err := b.checkCIDR(matched.Entry, req); err != nil {
return nil, err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the error a logical.ErrorResponse to indicate the cause of the problem as opposed to an internal error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, it will already be one because that's what the checkCIDR method will return if the check is unsuccessful.

}

clientCerts := req.Connection.ConnState.PeerCertificates
if len(clientCerts) == 0 {
return logical.ErrorResponse("no client certificate found"), nil
Expand Down Expand Up @@ -101,6 +106,7 @@ func (b *backend) pathLogin(ctx context.Context, req *logical.Request, data *fra
Alias: &logical.Alias{
Name: clientCerts[0].SerialNumber.String(),
},
BoundCIDRs: matched.Entry.BoundCIDRs,
},
}

Expand Down Expand Up @@ -152,6 +158,10 @@ func (b *backend) pathLoginRenew(ctx context.Context, req *logical.Request, d *f
return nil, nil
}

if err := b.checkCIDR(cert, req); err != nil {
return nil, err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That also will already be that error.

}

if !policyutil.EquivalentPolicies(cert.Policies, req.Auth.Policies) {
return nil, fmt.Errorf("policies have changed, not renewing")
}
Expand All @@ -160,6 +170,7 @@ func (b *backend) pathLoginRenew(ctx context.Context, req *logical.Request, d *f
resp.Auth.TTL = cert.TTL
resp.Auth.MaxTTL = cert.MaxTTL
resp.Auth.Period = cert.Period
resp.Auth.BoundCIDRs = cert.BoundCIDRs
return resp, nil
}

Expand Down Expand Up @@ -371,6 +382,33 @@ func (b *backend) checkForValidChain(chains [][]*x509.Certificate) bool {
return false
}

func (b *backend) checkCIDR(cert *CertEntry, req *logical.Request) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering that this function can be potentially used by all the backends that introduces bound_cidr option, we might want to pull this out as a helper, possibly in helper/cidrutil package? It would then also make sense to accept ([]*sockaddr.SockAddrMarshaler, *logical.Connection) as parameters instead of (*CertEntry, *logical.Request).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can do that. I was thinking I'd do it in the next iteration because it's not yet reused in this PR, but I can just do it now too.


if len(cert.BoundCIDRs) <= 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Len is never going to be below zero. I think == makes more sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine! A previous dev team I was on preferred it that way because it was "more defensive". Will update.

// short-circuit the below logic
return nil
}

var valid bool
remoteSockAddr, err := sockaddr.NewSockAddr(req.Connection.RemoteAddr)
if err != nil {
if b.Logger().IsDebug() {
b.Logger().Debug("could not parse remote addr into sockaddr", "error", err, "remote_addr", req.Connection.RemoteAddr)
}
return logical.ErrPermissionDenied
}
for _, cidr := range cert.BoundCIDRs {
if cidr.Contains(remoteSockAddr) {
valid = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than setting the bool, you could just return nil here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️

break
}
}
if !valid {
return logical.ErrPermissionDenied
}
return nil
}

// parsePEM parses a PEM encoded x509 certificate
func parsePEM(raw []byte) (certs []*x509.Certificate) {
for len(raw) > 0 {
Expand Down
5 changes: 5 additions & 0 deletions logical/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package logical
import (
"fmt"
"time"

"github.com/hashicorp/go-sockaddr"
)

// Auth is the resulting authentication information that is part of
Expand Down Expand Up @@ -69,6 +71,9 @@ type Auth struct {
// mappings groups for the group aliases in identity store. For all the
// matching groups, the entity ID of the user will be added.
GroupAliases []*Alias `json:"group_aliases" mapstructure:"group_aliases" structs:"group_aliases"`

// The set of CIDRs that this token can be used with
BoundCIDRs []*sockaddr.SockAddrMarshaler `json:"bound_cidrs"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also need to be added to logical/plugin/pb/backend.proto and subsequently to logical/plugin/translation.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for point that out! I will tackle this after #4501 is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't forget about this :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nm, you already did it.

}

func (a *Auth) GoString() string {
Expand Down
2 changes: 2 additions & 0 deletions vault/request_handling.go
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,7 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re

// If the response generated an authentication, then generate the token
if resp != nil && resp.Auth != nil {

var entity *identity.Entity
auth = resp.Auth

Expand Down Expand Up @@ -574,6 +575,7 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re
TTL: tokenTTL,
NumUses: auth.NumUses,
EntityID: auth.EntityID,
BoundCIDRs: auth.BoundCIDRs,
}

te.Policies = policyutil.SanitizePolicies(te.Policies, true)
Expand Down
6 changes: 5 additions & 1 deletion website/source/api/auth/cert/index.html.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,17 @@ Sets a CA cert and associated parameters in a role name.
as it is renewed it never expires unless `max_ttl` is also set, but the TTL
set on the token at each renewal is fixed to the value specified here. If this
value is modified, the token will pick up the new value at its next renewal.
- `bound_cidrs` `(string: "", or list: [])` – If set, restricts usage of the
certificates to client IPs falling within the range of the specified
CIDR(s).

### Sample Payload

```json
{
"certificate": "-----BEGIN CERTIFICATE-----\nMIIEtzCCA5+.......ZRtAfQ6r\nwlW975rYa1ZqEdA=\n-----END CERTIFICATE-----",
"display_name": "test"
"display_name": "test",
"bound_cidrs": ["127.0.0.1/32", "128.252.0.0/16"]
}
```

Expand Down