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

authz: End2End test for AuditLogger #6304

Merged
merged 23 commits into from Jun 1, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
304 changes: 304 additions & 0 deletions authz/grpc_audit_end2end_test.go
@@ -0,0 +1,304 @@
package authz

import (
"context"
"crypto/tls"
"crypto/x509"
"encoding/json"
"io"
"net"
"os"
"testing"
"time"

"google.golang.org/grpc"
"google.golang.org/grpc/authz/audit"
"google.golang.org/grpc/credentials"
testgrpc "google.golang.org/grpc/interop/grpc_testing"
testpb "google.golang.org/grpc/interop/grpc_testing"
"google.golang.org/grpc/testdata"

_ "google.golang.org/grpc/authz/audit/stdout"
)

type testServer struct {
testgrpc.UnimplementedTestServiceServer
}

func (s *testServer) UnaryCall(ctx context.Context, req *testpb.SimpleRequest) (*testpb.SimpleResponse, error) {
return &testpb.SimpleResponse{}, nil
}

type statAuditLogger struct {
Stat map[bool]int
gtcooke94 marked this conversation as resolved.
Show resolved Hide resolved
stdoutLogger audit.Logger
rockspore marked this conversation as resolved.
Show resolved Hide resolved
SpiffeIds []string
rockspore marked this conversation as resolved.
Show resolved Hide resolved
}

func (s *statAuditLogger) Log(event *audit.Event) {
s.stdoutLogger.Log(event)
if event.Authorized {
s.Stat[true]++
} else {
s.Stat[false]++
}
s.SpiffeIds = append(s.SpiffeIds, event.Principal)
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have EventContent, do we still need this field singled out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm checking EventContent only once but I'd still prefer to check SPIFFE every time

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any specific reason behind that? I thought if Spiffe ID is populated in one case, we can safely assume it's already there.

}

type loggerBuilder struct {
Stat map[bool]int
SpiffeIds []string
}

func (loggerBuilder) Name() string {
return "stat_logger"
}
func (lb *loggerBuilder) Build(audit.LoggerConfig) audit.Logger {
dfawley marked this conversation as resolved.
Show resolved Hide resolved
return &statAuditLogger{
Stat: lb.Stat,
gtcooke94 marked this conversation as resolved.
Show resolved Hide resolved
stdoutLogger: audit.GetLoggerBuilder("stdout_logger").Build(nil),
}
}

func (*loggerBuilder) ParseLoggerConfig(config json.RawMessage) (audit.LoggerConfig, error) {
return nil, nil
}

const spiffeId = "spiffe://foo.bar.com/client/workload/1"

func TestAuditLogger(t *testing.T) {
tests := map[string]struct {
dfawley marked this conversation as resolved.
Show resolved Hide resolved
authzPolicy string
wantAllows int
wantDenies int
}{
"No audit": {
authzPolicy: `{
"name": "authz",
"allow_rules":
[
dfawley marked this conversation as resolved.
Show resolved Hide resolved
{
"name": "allow_UnaryCall",
"request":
{
"paths":
[
"/grpc.testing.TestService/UnaryCall"
]
}
}
],
"audit_logging_options": {
"audit_condition": "NONE",
"audit_loggers": [
{
"name": "stat_logger",
"config": {},
"is_optional": false
}
]
}
}`,
wantAllows: 0,
wantDenies: 0,
dfawley marked this conversation as resolved.
Show resolved Hide resolved
},
"Allow All Deny Streaming - Audit All": {
authzPolicy: `{
"name": "authz",
"allow_rules":
[
{
"name": "allow_all",
"request": {
"paths":
[
"*"
]
}
}
],
"deny_rules":
[
{
"name": "deny_all",
"request": {
"paths":
[
"/grpc.testing.TestService/StreamingInputCall"
]
}
}
],
"audit_logging_options": {
"audit_condition": "ON_DENY_AND_ALLOW",
"audit_loggers": [
{
"name": "stat_logger",
"config": {},
"is_optional": false
}
]
}
}`,
wantAllows: 2,
wantDenies: 1,
},
"Allow Unary - Audit Allow": {
authzPolicy: `{
"name": "authz",
"allow_rules":
[
{
"name": "allow_UnaryCall",
"request":
{
"paths":
[
"/grpc.testing.TestService/UnaryCall"
]
}
}
],
"audit_logging_options": {
"audit_condition": "ON_ALLOW",
"audit_loggers": [
{
"name": "stat_logger",
"config": {},
"is_optional": false
}
]
}
}`,
wantAllows: 2,
wantDenies: 0,
},
"Allow Typo - Audit Deny": {
authzPolicy: `{
"name": "authz",
"allow_rules":
[
{
"name": "allow_UnaryCall",
"request":
{
"paths":
[
"/grpc.testing.TestService/UnaryCall_Z"
]
}
}
],
"audit_logging_options": {
"audit_condition": "ON_DENY",
"audit_loggers": [
{
"name": "stat_logger",
"config": {},
"is_optional": false
}
]
}
}`,
wantAllows: 0,
wantDenies: 3,
},
}

for name, test := range tests {
t.Run(name, func(t *testing.T) {
// Setup test statAuditLogger, gRPC test server with authzPolicy, unary and stream interceptors
lb := &loggerBuilder{
Stat: make(map[bool]int),
}
audit.RegisterLoggerBuilder(lb)
i, _ := NewStatic(test.authzPolicy)
cert, err := tls.LoadX509KeyPair(testdata.Path("x509/server1_cert.pem"), testdata.Path("x509/server1_key.pem"))
if err != nil {
t.Fatalf("tls.LoadX509KeyPair(x509/server1_cert.pem, x509/server1_key.pem) failed: %v", err)
}
ca, err := os.ReadFile(testdata.Path("x509/client_ca_cert.pem"))
if err != nil {
t.Fatalf("os.ReadFile(x509/client_ca_cert.pem) failed: %v", err)
}
certPool := x509.NewCertPool()
if !certPool.AppendCertsFromPEM(ca) {
t.Fatal("failed to append certificates")
}
creds := credentials.NewTLS(&tls.Config{
ClientAuth: tls.RequireAndVerifyClientCert,
Certificates: []tls.Certificate{cert},
ClientCAs: certPool,
})
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be the same in every test case... I would move it outside the loop. Optionally: same thing with the logger builder, unless you're concerned about state leaking.

s := grpc.NewServer(
grpc.Creds(creds),
grpc.ChainUnaryInterceptor(i.UnaryInterceptor),
grpc.ChainStreamInterceptor(i.StreamInterceptor))
defer s.Stop()
testgrpc.RegisterTestServiceServer(s, &testServer{})
Copy link
Member

Choose a reason for hiding this comment

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

Does testServer have some magic behavior you need?

I would greatly prefer the use of grpc/internal/stubserver instead. See grpc/test/* for example usages. It should be really easy to use and not couple the behavior of this test and the service's implementation which is in another location.

lis, err := net.Listen("tcp", "localhost:0")
gtcooke94 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
t.Fatalf("error listening: %v", err)
}
go s.Serve(lis)

// Setup gRPC test client with certificates containing a SPIFFE Id
cert, err = tls.LoadX509KeyPair(testdata.Path("x509/client_with_spiffe_cert.pem"), testdata.Path("x509/client_with_spiffe_key.pem"))
if err != nil {
t.Fatalf("tls.LoadX509KeyPair(x509/client1_cert.pem, x509/client1_key.pem) failed: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

This error message doesn't match the code.

Two options: 1. fix it, or 2. use a helper to fix this and everything else forever and always:

func loadKeys(t *testing.T, cert, key string) tls.WhateverTypeThisIs {
  t.Helper()
  cert, err := tls.LoadX509KeyPair(testdata.Path(cert), testdata.Path(key))
  if err != nil {
    t.Fatalf("tls.LoadX509KeyPair(%q, %q) failed: %v", cert, key, err)
  }
  return cert
}

}
ca, err = os.ReadFile(testdata.Path("x509/server_ca_cert.pem"))
if err != nil {
t.Fatalf("os.ReadFile(x509/server_ca_cert.pem) failed: %v", err)
}
roots := x509.NewCertPool()
if !roots.AppendCertsFromPEM(ca) {
t.Fatal("failed to append certificates")
}
creds = credentials.NewTLS(&tls.Config{
Certificates: []tls.Certificate{cert},
RootCAs: roots,
ServerName: "x.test.example.com",
})
clientConn, err := grpc.Dial(lis.Addr().String(), grpc.WithTransportCredentials(creds))
if err != nil {
t.Fatalf("grpc.Dial(%v) failed: %v", lis.Addr().String(), err)
}
defer clientConn.Close()
client := testgrpc.NewTestServiceClient(clientConn)
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()

//Make 2 unary calls and 1 streaming call
client.UnaryCall(ctx, &testpb.SimpleRequest{})
client.UnaryCall(ctx, &testpb.SimpleRequest{})
stream, err := client.StreamingInputCall(ctx)
if err != nil {
t.Fatalf("failed StreamingInputCall err: %v", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

Why do you check this error but not any error that could be returned by UnaryCall?

Also, starting a stream almost never fails. The way to get that error is to capture it from CloseAndRecv.

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 a good point - I think the nature of the test is literally testing audit part of authz policy so I think just making calls and ignoring errors is fine.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think ignoring errors is a good idea. Check them so we can more easily debug what went wrong instead of trying to figure out why the numbers didn't match up at the end.

req := &testpb.StreamingInputCallRequest{
Payload: &testpb.Payload{
Body: []byte("hi"),
},
}
if err := stream.Send(req); err != nil && err != io.EOF {
t.Fatalf("failed stream.Send err: %v", err)
}
stream.CloseAndRecv()
Copy link
Member

Choose a reason for hiding this comment

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

This needs to check the 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.

I'm leaning towards ignoring the errors here - #6304 (comment)


//Compare expected number of allows/denies with content of internal map of statAuditLogger
rockspore marked this conversation as resolved.
Show resolved Hide resolved
if lb.Stat[true] != test.wantAllows {
t.Errorf("Allow case failed, want %v got %v", test.wantAllows, lb.Stat[true])
}
if lb.Stat[false] != test.wantDenies {
t.Errorf("Deny case failed, want %v got %v", test.wantDenies, lb.Stat[false])
}
//Compare recorded SPIFFE Ids with the value from cert
for _, id := range lb.SpiffeIds {
if id != spiffeId {
t.Errorf("Unexpected SPIFFE Id, want %v got %v", spiffeId, id)
}
}
})
}
}
33 changes: 33 additions & 0 deletions testdata/x509/client_with_spiffe_cert.pem
@@ -0,0 +1,33 @@
-----BEGIN CERTIFICATE-----
MIIFxzCCA6+gAwIBAgICA+gwDQYJKoZIhvcNAQELBQAwUDELMAkGA1UEBhMCVVMx
CzAJBgNVBAgMAkNBMQwwCgYDVQQHDANTVkwxDTALBgNVBAoMBGdSUEMxFzAVBgNV
BAMMDnRlc3QtY2xpZW50X2NhMB4XDTIzMDUyMjA1MDA1NVoXDTMzMDUxOTA1MDA1
NVowTjELMAkGA1UEBhMCVVMxCzAJBgNVBAgMAkNBMQwwCgYDVQQHDANTVkwxDTAL
BgNVBAoMBGdSUEMxFTATBgNVBAMMDHRlc3QtY2xpZW50MTCCAiIwDQYJKoZIhvcN
AQEBBQADggIPADCCAgoCggIBANXyLXGYzQFwLGwjzkeuo/y41voDH1Y9J+ee4qJU
OFuMKKXx5ai7n7dik4//J12OqJbbr416cFkKmcojwwbAdncXMV58EF82Bt8QRov0
Vtoio/wxlyRlxDlVYwr56W+0EVP9Q+kzA/dTnMgOQYIeSix96CUQRy8XDu1YX3rk
fiUkND9xxuQw8OXi3LXguv/lilLVC/lXiXwa0RWEgMZZU2S1/lAElAG3aZuuWULG
K+PpKPuqkcptbUPCvNN1eUs9/D82aoFuqRCmpTC+7bUO+SJSggpUHcgTbXT9i6OO
9eR0ijcaQjtb0Y6ro+Cv60YOnlGC8It3KoY2SxioyqdceRUohqs4T4hjBEckzz11
AC0Pj0Gp4NJPcOY68EjhD5rvncn76RRr3z2XZpd+2Nz+Fldxk/aaejfdgqs9lo1g
C+aP+nk9oqSpFAc+rpHsblLZehUur/FHhenn1pYWqkSJsAG0sFW4sDHATRIfva3c
HNHB5kBzruGymywBGO0xOw7+s5XzPiNnbXT5FBY1rKG7RwlqdtDh6LWJRHmEblWV
tPHNiY+rrStv0rN7Hk/YKcSXd5JiTjk3GXjO1YJJVEraEWHlxzdGy+xu/m0iJLed
pxZwuxxdZ/Q2+Ht+X9pO2DsW8BQFbddCwbooxKygwSlmHCN1gRSWqWMZY5nzsxxY
tic9AgMBAAGjgawwgakwDAYDVR0TAQH/BAIwADAdBgNVHQ4EFgQUyiXne0d3g9zN
gjhCdl2d9ykxIfgwDgYDVR0PAQH/BAQDAgXgMBYGA1UdJQEB/wQMMAoGCCsGAQUF
BwMCMDEGA1UdEQQqMCiGJnNwaWZmZTovL2Zvby5iYXIuY29tL2NsaWVudC93b3Jr
bG9hZC8xMB8GA1UdIwQYMBaAFOr3a0MblN9W9Opu7VsDn3crpoDCMA0GCSqGSIb3
DQEBCwUAA4ICAQB3pt3mLXDDcReko9eTFahkNyU2zGP7CSi1RcgfP1aJDLBTjePb
JUhoY14tSpOGSliXWscXbNveW+Yk+tB411r8SJuXIkaYZM2BJQDWFzL7aLfAQSx5
rf8tHVyKO89uBoQtgEnxZp9BFhBp/b2y5DLrZWjM6W9s21C9S9UIFjD8UwrKicNH
HGxIC6AZ6yc0x2Nsq/KW1IZ6HDueZRB4tud75wwkPVpS5fb+XqIJEBP7lgYrJjGp
aLLxV2vn1kX2/qbH31hhWVpNyPkpFsT+IbkPFLDyQoZKHbewD6M56+KBRTTENETQ
hFLgJB0HiICJ2I6cqw1UbDJMJFkcnThsuI8Wg9dxZ+OffYeZ5bnFCVIg0WUi9oMK
JDXZAqYDwBaQHyNszaYzZ5VE2Gd/K8PEDevW4RblI+vAOamIM5w1DjQHWf7n1byt
nGwnxt4IQ5vwlrdX3FDcEkhacHdcniX/FTpYrfOistPh+QpBAvA92DG1CbAf2nKY
yXLx+Ho7tUEBGioU4XvRHccwumfatf5z+JO/EvIi2yWd1tanl5J3o/sSs9ixJfx4
aSuM+zAwf8EM+YGqYMCZ896+T6/r7NAg+YIDYu1K5b5QqYyPanqNqUf9VTR4oQ4v
+jdb5PkujXbjENvkAhNbUyUbQJ+IU0KHm3/sdhRPN5tuc9C+BTSQvlmKkw==
-----END CERTIFICATE-----
52 changes: 52 additions & 0 deletions testdata/x509/client_with_spiffe_key.pem
@@ -0,0 +1,52 @@
-----BEGIN PRIVATE KEY-----
MIIJQwIBADANBgkqhkiG9w0BAQEFAASCCS0wggkpAgEAAoICAQDV8i1xmM0BcCxs
I85HrqP8uNb6Ax9WPSfnnuKiVDhbjCil8eWou5+3YpOP/yddjqiW26+NenBZCpnK
I8MGwHZ3FzFefBBfNgbfEEaL9FbaIqP8MZckZcQ5VWMK+elvtBFT/UPpMwP3U5zI
DkGCHkosfeglEEcvFw7tWF965H4lJDQ/ccbkMPDl4ty14Lr/5YpS1Qv5V4l8GtEV
hIDGWVNktf5QBJQBt2mbrllCxivj6Sj7qpHKbW1DwrzTdXlLPfw/NmqBbqkQpqUw
vu21DvkiUoIKVB3IE210/YujjvXkdIo3GkI7W9GOq6Pgr+tGDp5RgvCLdyqGNksY
qMqnXHkVKIarOE+IYwRHJM89dQAtD49BqeDST3DmOvBI4Q+a753J++kUa989l2aX
ftjc/hZXcZP2mno33YKrPZaNYAvmj/p5PaKkqRQHPq6R7G5S2XoVLq/xR4Xp59aW
FqpEibABtLBVuLAxwE0SH72t3BzRweZAc67hspssARjtMTsO/rOV8z4jZ210+RQW
Nayhu0cJanbQ4ei1iUR5hG5VlbTxzYmPq60rb9Kzex5P2CnEl3eSYk45Nxl4ztWC
SVRK2hFh5cc3Rsvsbv5tIiS3nacWcLscXWf0Nvh7fl/aTtg7FvAUBW3XQsG6KMSs
oMEpZhwjdYEUlqljGWOZ87McWLYnPQIDAQABAoICAAY5tM7QZm67R9+hrxfw4f6x
ljfSLXBB+U5JFkko8DbhvjEN9+PQCda5PJf9EbUsOIWjQNl6DZjZsR3rqnog0ZGn
kB0yuPs8RDjrbVIXOwu/5EurWb2KZIpSjL4+BWflsndiMD6x6FSjDzXXDFrv7LKc
u0uQzLF3F00avDSEP5NvGUIbWnE7Z1cZIdj9ABQAJuVAI8gOnwaIdTsODv02jjGp
BgxoBbKDFsSb7yb9QzuvhizEitd8FajaGsqAaZYh6JwiRjkb8jl0z+u6MoqJNACm
q/gG+JLg1deIpS6OM2OBbKAr2G+HvXJMVklsdQkl1b+DcuJsBkW/gLHn/3WdQDyq
t9sB8XrUx3S5dy6oroj9tcrwwlpUPbx3/37BX7OEn/NlIWZojI62hGexoFaggu3O
Jg0JJYH8THlQqs9G/oXmRTQKngse2FLEIPePie9vIIvokiQtG4T6miOK+6QmxYZq
H+KPT8AQG8j7AEexo4is1mEayapmTxftIYANOLFaT82BhsUOZksa698Sz7k1Cf/o
MSFn6CocGLflMEzdBqEq0wbBkdeuKUKlXG3ztXlqElN1xFdbzkaP/Tzl1ooq3kLR
0cLBCJNrHxTo1tuW4qTn+s4GLHpM4PE4YZgMehVWtyRFBb7mrSXsESw03POvulBx
65vA86DtCPm/jVuC5lQBAoIBAQD8IWDHYtQnvn/za6etc0fc7eTyq1jmoS/gh33y
eHaY6IccUN2LXCxgTJYwmfy57De58Im5AcOnkgGvw2Hw2i6+A5i4tKkyCa9awW4A
M20QOnyQpF+9uiIqGzI72skpfH20SvgTstTFtgGr3UBOqTfcApL+1X4guvGnY+Cx
uHUAPzbis9G3CNOWb4iiLhUcBnTDZyB3MPM4S1U8E5JLFo86+va6gbqdBP4ac+KH
08XDk/z6ohp9p796o6IiBQyZEsVaYLCrzjSOXeFfE5Fyj2z53oGlws+/PdhXKo02
3++zRESiLVuGbCmAN17nKwDbZu9kFfGNP2WdwhJt9Yey91I9AoIBAQDZOsXWNkyP
zoDcSrvJznMPFcwQCbMNyU7A+axXpAsxDqn16AQj5/t1PWqufjRSdC7gVUWFcQ2K
ldUHkNyGtqHVCcNpqsMZJT51NlgTOl1A3GLnmm+tAiMCcr7aySeNnlj08fW278Ek
fnxpgUqGtXjTFpArULSFdZulXNPAP85ZDBburJtdhMfiT3GyQ1iRZcXkzsUVzNU1
nGGk0jtCodlzQKiz3/aHO63G0GAjtdPuXpzGm7nBJSgLD0GabkCdkHDFULOaraYy
t1zsCsg7tQWa4KGRDNkcJKzoz3zf1sI4g87UJceGoXdB+mfluyKtnFhqjFalFW8Y
14Yb8YYdYHkBAoIBAC1pZaED7+poqWsSjNT02pC0WHRM4GpJxfHO9aRihhnsZ8l1
1zFunJ+Lq9F9KsPiA/d9l5C2/KKF7b/WlSFoatrWkv9RqtfUXr0d8c4fdRljL2Rt
9sCZceXbmCSnt2u9fHaouh3yK9ige5SU+Swx1lnOLOOxWFJU2Ymot6PK8Wfl+uDC
OpeZA2MpG5b6bdrqXsWDIZnWOzh8eRGlBMh5e7rH0QCutQnrCEmDbd3BCvG7Cemq
oNLZD+fq6Rzvg+FePCWXHLsVHOo3how1XhEgPCSVKwzMFdcAMKMiiuTDWM0VEreT
K9T+TktFrdY9LJ5X3+5K9YLXVFohxmf/vT1CxpECggEBAIfegeVU+xgrYl/nAoPb
9A1oZcVWO78QvYhn4YrDmRhrApVDNGu86oPPEU3otBMqhjNcQmqPZpfa1W6xBa3g
x2H3hFkwLG0q5WDsx7PnGnK6JcaUyurcXkdmu8ceb/XdJ+i0+ioc1aJc1rYq3xFY
qiTlhPECvpaHE/4fDHa/sfHyZNmN7nNU3KzJYeTMyLXQgTF2vsC+6FBq6ovrzpMD
pn224I35NDorcqrapHdRgCgk10xGFK4g7mXUegT8lr+2m0JfEqdZm403MRCWQd1O
gR35CDUwYw9+RQQs2v8qVTqB/riklKK5lV0YISoInU0XcBncg0koGd/g1gneTDNN
pwECggEBAM4sDCCPplzbyd0yXLGo9P3RYIsNFnKnIm0YGRPrevBaiux3Qhr7Whpi
eV04BJ7Q58Z2WFzPFMhdXU45y4c6jIbmikdplEW1TASgXxOTvTfhg8P8ljdLPx+R
3CvQi4BPkJ3ZtYrHLKXKF/9aseyHLlSzuNUAJ6H0YxVi0tmzCFG82SWcFOzhR2Ec
cWDptGTRt9YY+Eo5rhPYbX/s8fCcW2u9QGnRnX35F8vJOp8Q7eCONIaN6faV4Yos
1wk6WXjZfDgEdjxmrnqXrgxdup82uD4Q1agmkxAjPl/9frLtHMW87Y0OixJb/Sve
eSCMKThlBQ57WubHTi2TbFBVKph/rP0=
-----END PRIVATE KEY-----