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

fix(policy): enhance proto validation across policy requests #1656

Merged
merged 28 commits into from
Nov 4, 2024

Conversation

jakedoublev
Copy link
Contributor

@jakedoublev jakedoublev commented Oct 16, 2024

Resolves #1640 and a host of similar issues with validation throughout policy service protos

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis
@jakedoublev
Copy link
Contributor Author

As part of this work I explored custom validators, which would look like:

// validator.proto
edition = "2023";

package policy;

import "buf/validate/validate.proto";

extend buf.validate.StringRules {
  bool optional_uuid = 70000001 [            // opens up custom validator '.string.(optional_uuid)'
    (buf.validate.predefined).cel = {
      id: "uuid_format",
      message: "field must be a valid UUID",
      expression: "size(this) == 0 || this.matches('[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}')"
    }
  ];
}

Then in any proto we have an optional UUID, we could do:

message ListKeyAccessServerGrantsRequest {
  // Optional
  string kas_id = 1 [(buf.validate.field).string.(optional_uuid) = true]; // custom validator
}

The custom validator feature updates we need were merged into protovalidate about a month ago and aren't supported by all our plugins yet (buf.build/grpc-ecosystem/gateway at the latest (v2.22.0) doesn't yet support "editions" which is a requirement 😞 ). Not only that, but it's also likely plugins for other languages won't support them either, so it's too bleeding edge for us.

We still improve validation of many fields in this PR, but it requires duplicate rules and CEL expressions we cannot yet unify with custom validations.

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis
@jakedoublev jakedoublev marked this pull request as ready for review October 16, 2024 22:24
@jakedoublev jakedoublev requested review from a team as code owners October 16, 2024 22:24

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis
@ryanulit
Copy link
Contributor

LGTM depending on your thoughts on my comments. Won't approve yet due to code freeze.

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis

Verified

This commit was signed with the committer’s verified signature.
jakedoublev Jake Van Vorhis
@jakedoublev jakedoublev enabled auto-merge November 4, 2024 19:46
@jakedoublev jakedoublev added this pull request to the merge queue Nov 4, 2024
Merged via the queue into main with commit df534c4 Nov 4, 2024
21 checks passed
@jakedoublev jakedoublev deleted the fix/protovalidate branch November 4, 2024 20:46
github-merge-queue bot pushed a commit that referenced this pull request Nov 5, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
🤖 I have created a release *beep* *boop*
---


##
[0.2.19](protocol/go/v0.2.18...protocol/go/v0.2.19)
(2024-11-05)


### Features

* **policy:** 1651 move GetAttributesByValueFqns RPC request validation
to protovalidate
([#1657](#1657))
([c7d6b15](c7d6b15))
* **policy:** add optional name field to registered KASes in policy
([#1636](#1636))
([f1382c1](f1382c1))
* **policy:** limit/offset throughout LIST protos/gencode
([#1668](#1668))
([7de6cce](7de6cce))
* **policy:** subject condition sets prune protos/gencode
([#1687](#1687))
([a627e02](a627e02))


### Bug Fixes

* **policy:** enhance proto validation across policy requests
([#1656](#1656))
([df534c4](df534c4))
* **policy:** make MatchSubjectMappings operator agnostic
([#1658](#1658))
([cb63819](cb63819))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
Co-authored-by: Jake Van Vorhis <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Nov 14, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
🤖 I have created a release *beep* *boop*
---


##
[0.4.27](service/v0.4.26...service/v0.4.27)
(2024-11-14)


### Features

* **authz:** JWT ERS that just returns claims
([#1630](#1630))
([316b5be](316b5be))
* **authz:** Remove org-admin role, move privileges to admin role
([#1740](#1740))
([ae931d0](ae931d0))
* backend migration to connect-rpc
([#1733](#1733))
([d10ba3c](d10ba3c))
* connectrpc realip interceptor
([#1728](#1728))
([292fca0](292fca0))
* **docs:** add policy ADR for LIST limit and pagination
([#1557](#1557))
([069f939](069f939))
* move from fasthttp in-memory listener to memhttp implementation
([#1709](#1709))
([70518ff](70518ff))
* **policy:** 1603 policy improve upsertattrfqn
([#1679](#1679))
([cd17a44](cd17a44))
* **policy:** 1651 move GetAttributesByValueFqns RPC request validation
to protovalidate
([#1657](#1657))
([c7d6b15](c7d6b15))
* **policy:** 1659 spike on transactions support
([#1678](#1678))
([a6fea11](a6fea11))
* **policy:** add optional name field to registered KASes in policy
([#1636](#1636))
([f1382c1](f1382c1))
* **policy:** add optional name field to registered KASes in policy
([#1641](#1641))
([b277ab4](b277ab4))
* **policy:** limit/offset throughout LIST protos/gencode
([#1668](#1668))
([7de6cce](7de6cce))
* **policy:** SPIKE transactions support
([#1663](#1663))
([866f4f3](866f4f3))
* **policy:** subject condition sets prune protos/gencode
([#1687](#1687))
([a627e02](a627e02))
* **policy:** subject condition sets prune service/db
([#1688](#1688))
([3cdd1b2](3cdd1b2)),
closes [#1178](#1178)
* update service registry in preperation for connectrpc migration
([#1715](#1715))
([ce289a4](ce289a4))


### Bug Fixes

* cleanup left over status.Error in favor of connect.NewError
([#1751](#1751))
([acea8d1](acea8d1))
* **core:** Autobump service
([#1726](#1726))
([39a898d](39a898d))
* **core:** Autobump service
([#1739](#1739))
([46662a7](46662a7))
* **core:** Autobump service
([#1750](#1750))
([4b239b1](4b239b1))
* Fixtures CodeQL alert for potentially unsafe quoting
([#1703](#1703))
([6f2fa9b](6f2fa9b))
* **kas:** Only hit authorization if data attributes not empty
([#1741](#1741))
([471f5f1](471f5f1))
* **policy:** enhance proto validation across policy requests
([#1656](#1656))
([df534c4](df534c4))
* **policy:** make MatchSubjectMappings operator agnostic
([#1658](#1658))
([cb63819](cb63819))
* **policy:** REVERT PR
[#1663](#1663) - SPIKE
transactions support
([#1719](#1719))
([184a733](184a733))
* **policy:** schema markdown links should work
([#1672](#1672))
([4122262](4122262))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Policy: KAS registry URI validation is invalid Policy: KAS registration lacks validation of URI
2 participants