authz: add audit logging APIs

[rockspore]

Implements interfaces for pending gRFC A59: https://github.com/grpc/proposal/pull/346/files

RELEASE NOTES: N/A

[rockspore]

One small thing to note is that I removed context.Context from the Log() method of the logger. It's not really needed and to be consistent with other languages, we will not offer the method any additional information than the AuditInfo.

[rockspore]

Regarding a separate auditlog pkg, that was the original proposal and was discussed offline. While I am pretty neutral on this, IIRC, the reasoning was that separate packages always added incremental costs and everything here would only be consumed within authz with no plan to add anything to make audit logging more standalone and separate from authorization. Therefore, although variable and func names have to be more verbose, putting everything in authz actually makes it clear and convenient for users.

[easwars]

Nit

Suggested change

	// RegisterAuditLoggerBuilder registers the builder in a global map
	// using b.Name() as the key.
	// This should only be called during initialization time (i.e. in an init()
	// function).
	// If multiple builders are registered with the same name, the one registered
	// last will take effect.
	// RegisterAuditLoggerBuilder registers the builder in a global map
	// using b.Name() as the key.
	//
	// This should only be called during initialization time (i.e. in an init()
	// function). If multiple builders are registered with the same name,
	// the one registered last will take effect.

[easwars]

How about?

Suggested change

	// AuditEvent contains information used by the audit logger during an audit
	// logging event.
	// AuditEvent contains information passed to the audit logger as part of
	// an audit logging event.

[easwars]

This would currently show up as follows in the godoc:

// AuditLoggerConfig defines the configuration for a particular implementation
// of audit logger.
type AuditLoggerConfig interface {
    // contains filtered or unexported methods
}

Consider changing this to:

// AuditLoggerConfig represents an opaque data structure holding an audit
// logger configuration. Concrete types representing configuration of specific
// audit loggers must embed this interface to implement it.
type AuditLoggerConfig interface {
	// auditLoggerConfig is a dummy interface requiring users to embed this
	// interface to implement it.
	auditLoggerConfig()
}

[easwars]

How about:

Suggested change

	// Log does audit logging with the given information in the audit event.
	// This method will be executed synchronously by gRPC so implementers must
	// keep in mind it must not block the RPC. Specifically, time-consuming
	// processes should be fired asynchronously such that this method can
	// return immediately.
	// Log performs audit logging for the provided audit event.
	//
	// This method is invoked in the RPC path and therefore implementations
	// must not block.

[easwars]

Suggested change

	// AuditLogger is the interface for an audit logger.
	// An audit logger is a logger instance that can be configured to use via the
	// authorization policy or xDS HTTP RBAC filters. When the authorization
	// decision meets the condition for audit, all the configured audit loggers'
	// Log() method will be invoked to log that event with the AuditInfo.
	// The method will be executed synchronously before the authorization is
	// complete and the call is denied or allowed.
	//
	// AuditLogger is the interface to be implemented by audit loggers.
	//
	// An audit logger is a logger instance that can be configured via the
	// authorization policy API or xDS HTTP RBAC filters. When the authorization
	// decision meets the condition for audit, all the configured audit loggers'
	// Log() method will be invoked to log that event.

Skipping the line about the method being invoked inline since the same is mentioned in the docstring for Log.

[easwars]

Suggested change

	// AuditLoggerBuilder is the interface for an audit logger builder.
	// It parses and validates a config, and builds an audit logger from the parsed
	// config. This enables configuring and instantiating audit loggers in the
	// runtime. Users that want to implement their own audit logging logic should
	// implement this along with the AuditLogger interface and register this
	// builder by calling RegisterAuditLoggerBuilder() before they start the gRPC
	// server.
	//
	// AuditLoggerBuilder is the interface to be implemented by audit logger
	// builders that are used at runtime to configure and instantiate audit loggers.
	//
	// Users who want to implement their own audit logging logic should
	// implement this interface, along with the AuditLogger interface, and register it
	// by calling RegisterAuditLoggerBuilder() at init time.
	//

[easwars]

This is mentioned in the docstring of the AuditLoggerConfig. I would consider skipping it here.

[rockspore]

Done.

[rockspore]

Thanks a lot for the suggested revision of those comments! They definitely document the APIs in a clearer way. I took all the suggestions. @easwars

[gtcooke94]

LGTM

[easwars]

Nit: these two lines can be removed.

[easwars]

@rockspore
How soon do you want this merged? I see that @dfawley had requested some changes and I think you have handled all of those. If this can wait till next week, then we can wait for him to come back, take another look and then merge. If this needs to go in asap, I will have to dismiss his review to be able to merge this. Let me know how you want to proceed. Thanks.

[rockspore]

@rockspore How soon do you want this merged? I see that @dfawley had requested some changes and I think you have handled all of those. If this can wait till next week, then we can wait for him to come back, take another look and then merge. If this needs to go in asap, I will have to dismiss his review to be able to merge this. Let me know how you want to proceed. Thanks.

Thanks. I'll leave this to @gtcooke94 who will follow up with the rest of work in Go.

[gtcooke94]

I'm good to wait for @dfawley, shouldn't be a diff of more than a few days