-
Notifications
You must be signed in to change notification settings - Fork 24
feat(policy): namespace root certificates #2771
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
base: main
Are you sure you want to change the base?
Conversation
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
…or certificate management. Add associated tests.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
service/policy/db/migrations/20251002000000_add_namespace_certificates.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple small comments.
Are you wanting to be able to retrieve your created certs? In addition, you will probably want an easy command to list your namespace <-> cert pairs. Key management uses a function called ListKeyMappings
, which will return all mappings for namespace/definition/value <-> kas asymmetric keys.
…distinction, enhance namespace-certificate management with ID/FQN support. Update related migrations, RPCs, and documentation.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Looks good. Gonna have @jakedoublev review it as well, just to make sure his concerns are met also. |
|
||
-- name: createCertificate :one | ||
INSERT INTO certificates (x5c, is_root, metadata) | ||
VALUES (sqlc.arg('x5c'), COALESCE(sqlc.narg('is_root')::BOOLEAN, TRUE), sqlc.arg('metadata')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we taking in is_root
from the protos at all? Or is every cert creation/assignment a root at this time? I didn't see it in the protos. 🤔
service/policy/db/migrations/20251002000000_add_namespace_certificates.sql
Show resolved
Hide resolved
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This is a great feature addition, enabling namespace-scoped root certificates. The implementation is well-structured, with good use of database transactions, clear separation of concerns, and appropriate validation. The addition of database migrations, documentation, and tests is also excellent.
I've identified a couple of areas for improvement related to database query efficiency and maintainability. My comments are detailed below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reviewing this pr something we should think about is tracking key fingerprints so we don't end up with duplicate keys in our system as well.
…Cs, migrations, documentation, and tests for consistency with PEM standards.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
…space # Conflicts: # docs/openapi/policy/attributes/attributes.openapi.yaml # protocol/go/policy/namespaces/namespaces.pb.go
…e certificate RPCs for enhanced certificate management.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
…n namespace key and certificate assignments
…in migration rollback
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
…pecs, and database mappings; update related documentation and RPCs.
Co-authored-by: Jake Van Vorhis <83739412+jakedoublev@users.noreply.github.com>
…d error context serialization
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
…nhanced context in unknown identifier cases
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
…ries, methods, and documentation for simplified root-only certificate management.
I added a check to see if the PEM is already added to the certificate table. so no duplicate PEM |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
…, and ensure proper certificate cleanup on namespace removal
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
This pull request introduces new support for namespace root certificates within the policy service, updates the API to expose these certificates, and makes related improvements to authentication and routing. The most significant changes include extending the
Namespace
object to include root certificates, updating the API and HTTP routing to allow public access to namespace information, and adding a proof-of-concept implementation for injecting a root CA certificate.http://localhost:8080/policy.namespaces.NamespaceService/GetNamespace
Namespace Root Certificate Support:
root_certs
repeated field of typeCertificate
to theNamespace
message inpolicy/objects.proto
, and defined a newCertificate
message to hold certificate data in PEM format.