Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ BUG FIXES:
* serviceregistration: Fix a regression for Consul service registration that ignored using the listener address as
the redirect address unless api_addr was provided. It now properly uses the same redirect address as the one
used by Vault's Core object. [[GH-8976](https://github.com/hashicorp/vault/pull/8976)]
* storage/raft: Advertise the configured cluster address to the rest of the nodes in the raft cluster. This fixes
an issue where a node advertising 0.0.0.0 is not using a unique hostname. [[GH-9008](https://github.com/hashicorp/vault/pull/9008)]
* sys: The path provided in `sys/internal/ui/mounts/:path` is now namespace-aware. This fixes an issue
with `vault kv` subcommands that had namespaces provided in the path returning permission denied all the time.
[[GH-8962](https://github.com/hashicorp/vault/pull/8962)]
Expand Down
30 changes: 21 additions & 9 deletions physical/raft/streamlayer.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"math/big"
mathrand "math/rand"
"net"
"net/url"
"sync"
"time"

Expand Down Expand Up @@ -135,13 +136,15 @@ func GenerateTLSKey(reader io.Reader) (*TLSKey, error) {
}, nil
}

// Make sure raftLayer satisfies the raft.StreamLayer interface
var _ raft.StreamLayer = (*raftLayer)(nil)
var (
// Make sure raftLayer satisfies the raft.StreamLayer interface
_ raft.StreamLayer = (*raftLayer)(nil)

// Make sure raftLayer satisfies the cluster.Handler and cluster.Client
// interfaces
var _ cluster.Handler = (*raftLayer)(nil)
var _ cluster.Client = (*raftLayer)(nil)
// Make sure raftLayer satisfies the cluster.Handler and cluster.Client
// interfaces
_ cluster.Handler = (*raftLayer)(nil)
_ cluster.Client = (*raftLayer)(nil)
)

// RaftLayer implements the raft.StreamLayer interface,
// so that we can use a single RPC layer for Raft and Vault
Expand Down Expand Up @@ -170,12 +173,21 @@ type raftLayer struct {
// from the network config.
func NewRaftLayer(logger log.Logger, raftTLSKeyring *TLSKeyring, clusterListener cluster.ClusterHook) (*raftLayer, error) {
clusterAddr := clusterListener.Addr()
switch {
case clusterAddr == nil:
// Clustering disabled on the server, don't try to look for params
if clusterAddr == nil {
return nil, errors.New("no raft addr found")
}

{
// Test the advertised address to make sure it's not an unspecified IP
u := url.URL{
Host: clusterAddr.String(),
}
ip := net.ParseIP(u.Hostname())
if ip != nil && ip.IsUnspecified() {
return nil, fmt.Errorf("cannot use unspecified IP with raft storage: %s", clusterAddr.String())
}
}

layer := &raftLayer{
addr: clusterAddr,
connCh: make(chan net.Conn),
Expand Down
70 changes: 70 additions & 0 deletions physical/raft/streamlayer_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
package raft

import (
"context"
"crypto/rand"
"crypto/tls"
"net"
"testing"
"time"

"github.com/hashicorp/vault/vault/cluster"
)

type mockClusterHook struct {
address net.Addr
}

func (*mockClusterHook) AddClient(alpn string, client cluster.Client) {}
func (*mockClusterHook) RemoveClient(alpn string) {}
func (*mockClusterHook) AddHandler(alpn string, handler cluster.Handler) {}
func (*mockClusterHook) StopHandler(alpn string) {}
func (*mockClusterHook) TLSConfig(ctx context.Context) (*tls.Config, error) { return nil, nil }
func (m *mockClusterHook) Addr() net.Addr { return m.address }
func (*mockClusterHook) GetDialerFunc(ctx context.Context, alpnProto string) func(string, time.Duration) (net.Conn, error) {
return func(string, time.Duration) (net.Conn, error) {
return nil, nil
}
}

func TestStreamLayer_UnspecifiedIP(t *testing.T) {
m := &mockClusterHook{
address: &cluster.NetAddr{
Host: "0.0.0.0:8200",
},
}

raftTLSKey, err := GenerateTLSKey(rand.Reader)
if err != nil {
t.Fatal(err)
}

raftTLS := &TLSKeyring{
Keys: []*TLSKey{raftTLSKey},
ActiveKeyID: raftTLSKey.ID,
}

layer, err := NewRaftLayer(nil, raftTLS, m)
if err == nil {
t.Fatal("expected error")
}

if err.Error() != "cannot use unspecified IP with raft storage: 0.0.0.0:8200" {
t.Fatalf("unexpected error: %s", err.Error())
}

if layer != nil {
t.Fatal("expected nil layer")
}

m.address.(*cluster.NetAddr).Host = "10.0.0.1:8200"

layer, err = NewRaftLayer(nil, raftTLS, m)
if err != nil {
t.Fatal(err)
}

if layer == nil {
t.Fatal("nil layer")
}
}
5 changes: 5 additions & 0 deletions vault/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,11 @@ func (c *Core) startClusterListener(ctx context.Context) error {
// If we listened on port 0, record the port the OS gave us.
c.clusterAddr.Store(fmt.Sprintf("https://%s", c.getClusterListener().Addr()))
Copy link
Contributor

@calvn calvn May 15, 2020

Choose a reason for hiding this comment

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

Could this result in raft using a bad listener address if the cluster_addr is, say 127.0.0.1:0 (a valid address with an unspecified port), and the address in the listener is 0.0.0.0:8201 since we'd be overwriting c.clusterAddr with one that raft is unhappy about?

}

if err := c.getClusterListener().SetAdvertiseAddr(c.ClusterAddr()); err != nil {
return err
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused on how this works. c.getClusterListener().Addr() returns back cl.Addrs()[0] the first time around, so could we simply do the parsing of the address into an absolute one and return that as part of Addr() instead of adding a new field and setter just for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is primarily necessary because our tests use random port assignments, so we actually don't know the port until we start the listener. If we didn't do this here then each test node would be advertising 127.0.0.1:0

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. My only concern here is that the logic right above could be overwriting c.clusterAddr to be a value a non-specified IP address value.

The call to c.getClusterListener().Addr()) could get us back an address from the listener config that's 0.0.0.0:8201 for instance, so we'd be updating c.clusterAddr to be https://0.0.0.0:8201.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least raft will then reject the IP. The user will need to re-configure it such that one of the two values is a fully formed hostname.

Copy link
Contributor

Choose a reason for hiding this comment

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

True. I was trying to think of other potential alternatives to this logic, since c.clusterAddr.Store(fmt.Sprintf("https://%s", c.getClusterListener().Addr())) followed by c.getClusterListener().SetAdvertiseAddr(c.ClusterAddr()) felt a little weird. IIUC it's taking the listener's address to update the cluster address and then the listener's address is updated with this cluster address, but it all seemed a little loopy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a little indirect, but i don't see a great way around that at the moment. Potentially we can push the :0 port check into test code since that's where it's needed.

return nil
}

Expand Down
31 changes: 31 additions & 0 deletions vault/cluster/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ import (
"errors"
"fmt"
"net"
"net/url"
"sync"
"sync/atomic"
"time"

"github.com/hashicorp/errwrap"
log "github.com/hashicorp/go-hclog"
"github.com/hashicorp/vault/sdk/helper/consts"
"golang.org/x/net/http2"
Expand Down Expand Up @@ -66,6 +68,7 @@ type Listener struct {

networkLayer NetworkLayer
cipherSuites []uint16
advertise net.Addr
logger log.Logger
l sync.RWMutex
}
Expand Down Expand Up @@ -94,7 +97,23 @@ func NewListener(networkLayer NetworkLayer, cipherSuites []uint16, logger log.Lo
}
}

func (cl *Listener) SetAdvertiseAddr(addr string) error {
u, err := url.ParseRequestURI(addr)
if err != nil {
return errwrap.Wrapf("failed to parse advertise address: {{err}}", err)
}
cl.advertise = &NetAddr{
Host: u.Host,
}

return nil
}

func (cl *Listener) Addr() net.Addr {
if cl.advertise != nil {
return cl.advertise
}

addrs := cl.Addrs()
if len(addrs) == 0 {
return nil
Expand Down Expand Up @@ -422,3 +441,15 @@ type NetworkLayer interface {
type NetworkLayerSet interface {
Layers() []NetworkLayer
}

type NetAddr struct {
Host string
}

func (c *NetAddr) String() string {
return c.Host
}

func (*NetAddr) Network() string {
return "tcp"
}
10 changes: 9 additions & 1 deletion vault/cluster/tcp_layer.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package cluster
import (
"crypto/tls"
"net"
"strings"
"sync"
"time"

Expand Down Expand Up @@ -64,7 +65,14 @@ func (l *TCPLayer) Listeners() []NetworkListener {
l.logger.Info("starting listener", "listener_address", laddr)
}

tcpLn, err := net.ListenTCP("tcp", laddr)
// If they've passed 0.0.0.0, we only want to bind on IPv4
// rather than golang's dual stack default
bindProto := "tcp"
if strings.HasPrefix(laddr.String(), "0.0.0.0:") {
bindProto = "tcp4"
}

tcpLn, err := net.ListenTCP(bindProto, laddr)
if err != nil {
l.logger.Error("error starting listener", "error", err)
continue
Expand Down
2 changes: 1 addition & 1 deletion vault/core.go
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,7 @@ type Core struct {
// Stop channel for raft TLS rotations
raftTLSRotationStopCh chan struct{}
// Stores the pending peers we are waiting to give answers
pendingRaftPeers map[string][]byte
pendingRaftPeers *sync.Map

// rawConfig stores the config as-is from the provided server configuration.
rawConfig *atomic.Value
Expand Down
13 changes: 8 additions & 5 deletions vault/logical_system_raft.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,17 @@ func (b *SystemBackend) handleRaftBootstrapChallengeWrite() framework.OperationF
return logical.ErrorResponse("no server id provided"), logical.ErrInvalidRequest
}

answer, ok := b.Core.pendingRaftPeers[serverID]
var answer []byte
answerRaw, ok := b.Core.pendingRaftPeers.Load(serverID)
if !ok {
var err error
answer, err = uuid.GenerateRandomBytes(16)
if err != nil {
return nil, err
}
b.Core.pendingRaftPeers[serverID] = answer
b.Core.pendingRaftPeers.Store(serverID, answer)
} else {
answer = answerRaw.([]byte)
}

sealAccess := b.Core.seal.GetAccess()
Expand Down Expand Up @@ -243,14 +246,14 @@ func (b *SystemBackend) handleRaftBootstrapAnswerWrite() framework.OperationFunc
return logical.ErrorResponse("could not base64 decode answer"), logical.ErrInvalidRequest
}

expectedAnswer, ok := b.Core.pendingRaftPeers[serverID]
expectedAnswerRaw, ok := b.Core.pendingRaftPeers.Load(serverID)
if !ok {
return logical.ErrorResponse("no expected answer for the server id provided"), logical.ErrInvalidRequest
}

delete(b.Core.pendingRaftPeers, serverID)
b.Core.pendingRaftPeers.Delete(serverID)

if subtle.ConstantTimeCompare(answer, expectedAnswer) == 0 {
if subtle.ConstantTimeCompare(answer, expectedAnswerRaw.([]byte)) == 0 {
return logical.ErrorResponse("invalid answer given"), logical.ErrInvalidRequest
}

Expand Down
2 changes: 1 addition & 1 deletion vault/raft.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ func (c *Core) startRaftStorage(ctx context.Context) (retErr error) {
}

func (c *Core) setupRaftActiveNode(ctx context.Context) error {
c.pendingRaftPeers = make(map[string][]byte)
c.pendingRaftPeers = &sync.Map{}
return c.startPeriodicRaftTLSRotate(ctx)
}

Expand Down