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

auditloggingccl: migrate role-based audit logging as a CCL feature #104383

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

THardy98
Copy link

@THardy98 THardy98 commented Jun 6, 2023

Part of: #100467

This change moves the existing role-based audit logging logic to be
consumed as a CCL (enterprise) feature.

The notable changes here are:

  • moving the audit logging cluster settings (i.e. sql.log.user_audit
    and sql.log.user_audit.reduced_config.enabled) to the ccl package.
    Consequently, these cluster settings will only exist when the use has a
    CCL license (free or paid)
  • gating the sql.log.user_audit cluster setting behind the enterprise
    CCL license. This was done by adding an enterprise CCL license check
    in the sql.log.user_audit validation function. Users will be unable to
    change this cluster setting (and thereby will not be able to enable/configure
    role-based audit logging) unless they have an enterprise CCL license
  • the addition of function hooks to be used at CCL initialization,
    namely:
    • ConfigureRoleBasedAuditClusterSettings: used to add a
      SetOnChange hook to sql.log.user_audit
    • UserAuditLogConfigEmpty: used to check whether the audit logging
      cluster setting is empty
    • UserAuditReducedConfigEnabled: used to check whether the reduced
      audit configuration is enabled (note: regular users are still able
      to enable/disable this cluster setting, but it will take no effect as
      they will have no way to enable role-based audit logging)
    • UserAuditEnterpriseParamsHook: used to retrieve parameters
      necessary for enterprise license checks within the the
      sql.log.user_audit cluster setting validation function

Release note (sql change): Role-based audit logging is now a CCL
(enterprise) feature. Only enterprise CCL users will be able to
configure role-based audit logging using the sql.log.user_audit
cluster setting.

@THardy98 THardy98 marked this pull request as draft June 6, 2023 03:03
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@THardy98 THardy98 added release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. branch-release-23.1 Used to mark GA and release blockers, technical advisories, and bugs for 23.1 labels Jun 6, 2023
@THardy98 THardy98 force-pushed the ccl_audit_logging branch 3 times, most recently from 17491eb to c921005 Compare June 6, 2023 03:21
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 6 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @THardy98)


-- commits line 10 at r1:
make sure to fix the commit message


pkg/sql/auditlogging/audit_log.go line 17 at r1 (raw file):

	"fmt"
	"github.com/cockroachdb/cockroach/pkg/settings"
	"github.com/cockroachdb/cockroach/pkg/settings/cluster"

nit: location of these should be below


pkg/ccl/auditloggingccl/audit_log_config.go line 15 at r1 (raw file):

import (
	"context"
	"github.com/cockroachdb/cockroach/pkg/sql/auditlogging"

nit: the location of the import needs to be aligned with other imports

@THardy98 THardy98 force-pushed the ccl_audit_logging branch 4 times, most recently from e23d010 to 11e5249 Compare June 6, 2023 19:40
@THardy98 THardy98 requested review from dhartunian and HonoreDB and removed request for HonoreDB June 6, 2023 19:41
@THardy98
Copy link
Author

THardy98 commented Jun 6, 2023

Tagged @dhartunian based on our discussion this morning. I added the enterprise check such that one can change the cluster setting but it won't affect the underlying config if you do not have the paid enterprise license.

@HonoreDB verified that an enterprise check is indeed what's needed to gate the paid license.

@THardy98 THardy98 marked this pull request as ready for review June 6, 2023 19:44
@THardy98 THardy98 requested a review from a team as a code owner June 6, 2023 19:44
@THardy98 THardy98 force-pushed the ccl_audit_logging branch 4 times, most recently from 6d11849 to bd79115 Compare June 6, 2023 22:13
@THardy98 THardy98 force-pushed the ccl_audit_logging branch 2 times, most recently from 60eb8e8 to 9ddaf8f Compare June 6, 2023 22:56
@THardy98 THardy98 requested review from knz and maryliag June 7, 2023 01:09
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 9 files at r2, 5 of 10 files at r3, 8 of 8 files at r4, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @knz, and @THardy98)


pkg/ccl/auditloggingccl/audit_logging_test.go line 9 at r4 (raw file):

//     https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt

package auditloggingccl

any test that can be added to confirm the license?

@THardy98 THardy98 force-pushed the ccl_audit_logging branch from 9ddaf8f to 662f35d Compare June 7, 2023 13:51
@THardy98 THardy98 force-pushed the ccl_audit_logging branch from 662f35d to aa54198 Compare June 7, 2023 14:23
@maryliag
Copy link
Contributor

maryliag commented Jun 7, 2023

Changes :lgtm: , but I would like to see an approval from someone else that have knowledge on this area (e.g. @dhartunian @HonoreDB as you mentioned on this PR)

Copy link
Author

@THardy98 THardy98 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian, @knz, and @maryliag)


-- commits line 10 at r1:

Previously, maryliag (Marylia Gutierrez) wrote…

make sure to fix the commit message

Done.


pkg/sql/auditlogging/audit_log.go line 17 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: location of these should be below

As in the import order? If so, done.


pkg/ccl/auditloggingccl/audit_log_config.go line 15 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: the location of the import needs to be aligned with other imports

Done.


pkg/ccl/auditloggingccl/audit_logging_test.go line 9 at r4 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

any test that can be added to confirm the license?

Added TestRoleBasedAuditEnterpriseGated

@THardy98 THardy98 force-pushed the ccl_audit_logging branch from aa54198 to b46781f Compare June 7, 2023 14:43
Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

code :lgtm:

Just have some nits/questions which are non-blocking.

Reviewed 1 of 6 files at r1, 2 of 9 files at r2, 4 of 10 files at r3, 4 of 8 files at r4, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @knz, @maryliag, and @THardy98)


pkg/ccl/auditloggingccl/audit_log_config.go line 46 at r7 (raw file):

		"Users will need to start a new session to see these changes in their auditing behaviour.",
	false,
).WithPublic()

is there a reason these are no longer public? I assume we do want them to be documented and available for CCL users to use.


pkg/ccl/auditloggingccl/audit_log_config.go line 57 at r7 (raw file):

		return nil
	}
	st, clusterID, err := auditlogging.UserAuditEnterpriseParamsHook()

nit: not blocking but maybe in a follow-up you can use the *settings.Values that's being ignored up above in the function args instead of the one from this call.

This change moves the existing role-based audit logging logic to be
consumed as a CCL (enterprise) feature.

The notable changes here are:
- moving the audit logging cluster settings (i.e. `sql.log.user_audit`
  and `sql.log.user_audit.reduced_config.enabled`) to the `ccl` package.
Consequently, these cluster settings will only exist when the use has a
CCL license
- gating the `sql.log.user_audit` cluster setting behind the enterprise
  CCL license. This was done by adding an enterprise CCL license check
in the cluster setting's validation function. Users will be unable to
change this cluster setting (and thereby will not be able to enable
role-based audit logging) unless they have an enterprise CCL license
-  the addition of function hooks to be used on CCL initialization,
   namely:
   - `ConfigureRoleBasedAuditClusterSettings`: used to add the
     `SetOnChange` cluster setting hook to the audit logging cluster
setting
   - `UserAuditLogConfigEmpty`: used to check whether the audit logging
     cluster setting is empty
   - `UserAuditReducedConfigEnabled`: used to check whether the reduced
     audit configuration is enabled (note: regular users are still able
to enable/disable this cluster setting, but it will take no effect as
they will have no way to enable role-based audit logging)
   - `UserAuditEnterpriseParamsHook`: used to retrieve parameters
     necessary for enterprise license checks within the the
`sql.log.user_audit` cluster setting validation function

Release note (sql change): Role-based audit logging is now a CCL
(enterprise) feature. Only enterprise CCL users will be able to
configure role-based audit logging using the `sql.log.user_audit`
cluster setting.
@THardy98 THardy98 force-pushed the ccl_audit_logging branch from b46781f to f07d338 Compare June 8, 2023 00:07
Copy link
Author

@THardy98 THardy98 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @dhartunian, @knz, and @maryliag)


pkg/ccl/auditloggingccl/audit_log_config.go line 46 at r7 (raw file):

Previously, dhartunian (David Hartunian) wrote…

is there a reason these are no longer public? I assume we do want them to be documented and available for CCL users to use.

No, forgot I had removed this during revisions. Added back.

@THardy98
Copy link
Author

THardy98 commented Jun 8, 2023

TYFR

@THardy98
Copy link
Author

THardy98 commented Jun 8, 2023

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 8, 2023

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-release-23.1 Used to mark GA and release blockers, technical advisories, and bugs for 23.1 release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants