This repository was archived by the owner on Apr 6, 2026. It is now read-only.
NOISSUE - Unify prism certs#154
Merged
Merged
Conversation
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> update' Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> update sdk and tests Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> update cli Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> update certs client Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> rebase Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> update certs Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> update certs Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> update mocks Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
caeac28 to
2347087
Compare
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
SammyOina
suggested changes
Sep 4, 2025
| revokeCerts endpoint.Endpoint | ||
| retrieveCert endpoint.Endpoint | ||
| retrieveDownloadToken endpoint.Endpoint | ||
| issueCert endpoint.Endpoint |
Contributor
There was a problem hiding this comment.
why not use the exposed http api for this through sdk
Contributor
There was a problem hiding this comment.
same with the additional methods not used internally that require user tokens, we don't want private grpc for them
|
|
||
| // IssueCert issues a certificate from the database. | ||
| IssueCert(ctx context.Context, entityID, ttl string, ipAddrs []string, option SubjectOptions) (Certificate, error) | ||
| IssueCert(ctx context.Context, entityID, entityType, ttl string, ipAddrs []string, option SubjectOptions) (Certificate, error) |
Contributor
There was a problem hiding this comment.
there's only one entity type so we don't need the parameter
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
449295f to
e7aa7af
Compare
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
SammyOina
suggested changes
Sep 8, 2025
| timeout time.Duration | ||
| getEntityID endpoint.Endpoint | ||
| revokeCerts endpoint.Endpoint | ||
| getCA endpoint.Endpoint |
Contributor
There was a problem hiding this comment.
this is not required internally and cone be done via sdk
| } | ||
| } | ||
|
|
||
| func issueEndpoint(svc certs.Service) endpoint.Endpoint { |
Comment on lines
+21
to
+26
| getEntity kitgrpc.Handler | ||
| revokeCerts kitgrpc.Handler | ||
| retrieveCert kitgrpc.Handler | ||
| retrieveDownloadToken kitgrpc.Handler | ||
| issueCert kitgrpc.Handler | ||
| getCA kitgrpc.Handler |
Contributor
There was a problem hiding this comment.
keep only internal methods
Comment on lines
+17
to
+21
| rpc RetrieveCert(RetrieveCertReq) returns (CertificateBundle) {} | ||
| rpc RetrieveCertDownloadToken(RetrieveCertDownloadTokenReq) returns ( | ||
| RetrieveCertDownloadTokenRes) {} | ||
| rpc IssueCert(IssueCertReq) returns (IssueCertRes) {} | ||
| rpc GetCA(GetCAReq) returns (Cert) {} |
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
SammyOina
suggested changes
Sep 8, 2025
| IpAddrs []string `json:"ip_addresses"` | ||
| Options certs.SubjectOptions `json:"options"` | ||
| entityID string `json:"-"` | ||
| entityType string `json:"entity_type"` |
Contributor
There was a problem hiding this comment.
deprecated we only have one entity type, clean up the pr
Comment on lines
+275
to
+278
| // Set default entityType if not provided | ||
| if req.entityType == "" { | ||
| req.entityType = "backend" // Default to backend entity type | ||
| } |
Comment on lines
+31
to
+81
| message Cert { | ||
| string serial_number = 1; | ||
| string certificate = 2; | ||
| string key = 3; | ||
| bool revoked = 4; | ||
| google.protobuf.Timestamp expiry_time = 5; | ||
| string entity_id = 6; | ||
| string download_url = 7; | ||
| } | ||
|
|
||
| message CertificateBundle { | ||
| bytes ca = 1; | ||
| bytes certificate = 2; | ||
| bytes private_key = 3; | ||
| } | ||
|
|
||
| message RetrieveCertReq { | ||
| string download_token = 1; | ||
| string serial_number = 2; | ||
| } | ||
|
|
||
| message RetrieveCertDownloadTokenReq { | ||
| string domain_id = 1; | ||
| string serial_number = 2; | ||
| string user_id = 3; | ||
| } | ||
|
|
||
| message RetrieveCertDownloadTokenRes { | ||
| string token = 1; | ||
| } | ||
|
|
||
| message Options { | ||
| string common_name = 1; | ||
| repeated string organizations = 2; | ||
| repeated string organizational_units = 3; | ||
| repeated string countries = 4; | ||
| repeated string provinces = 5; | ||
| repeated string localities = 6; | ||
| repeated string street_addresses = 7; | ||
| repeated string postal_codes = 8; | ||
| repeated string dns_names = 9; | ||
| repeated string ip_addresses = 10; | ||
| } | ||
|
|
||
| message IssueCertReq { | ||
| string domain_id = 1; | ||
| string entity_id = 2; | ||
| string entity_type = 3; | ||
| repeated string ip_addresses = 4; | ||
| string user_id = 5; | ||
| string ttl = 6; |
Contributor
There was a problem hiding this comment.
remove deadcode and cleanup the pr
| for _, tc := range testCases { | ||
| t.Run(tc.desc, func(t *testing.T) { | ||
| agentCall := agent.On("Issue", tc.entityID, tc.ttl, []string{}, certs.SubjectOptions{}).Return(tc.cert, tc.agentErr) | ||
| // Expected SubjectOptions with defaults set by service |
Contributor
There was a problem hiding this comment.
remove unecessary comments
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
SammyOina
suggested changes
Sep 8, 2025
Comment on lines
+19
to
+23
| const ( | ||
| EntityTypeBackend EntityType = "backend" | ||
| EntityTypeCVM EntityType = "cvm" | ||
| CommonName = "Ultraviolet" | ||
| ) |
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
SammyOina
suggested changes
Sep 8, 2025
| root.SetArgs(args) | ||
| err := root.Execute() | ||
| assert.NoError(t, err, "Error executing command") | ||
| _ = root.Execute() |
Contributor
There was a problem hiding this comment.
why are you ignoring the error
| for _, tc := range cases { | ||
| t.Run(tc.desc, func(t *testing.T) { | ||
| svcCall := svc.On("IssueCert", mock.Anything, tc.entityID, tc.ttl, tc.ipAddrs, mock.Anything).Return(tc.svcresp, tc.svcerr) | ||
| svcCall := svc.On("IssueCert", mock.Anything, tc.entityID, "backend", tc.ttl, tc.ipAddrs, mock.Anything).Return(tc.svcresp, tc.svcerr) |
Contributor
There was a problem hiding this comment.
entity type is no longer used
| if tc.err == nil { | ||
| assert.Equal(t, tc.sdkCert.SerialNumber, resp.SerialNumber) | ||
| ok := svcCall.Parent.AssertCalled(t, "IssueCert", mock.Anything, tc.entityID, tc.ttl, tc.ipAddrs, certs.SubjectOptions{CommonName: tc.commonName}) | ||
| ok := svcCall.Parent.AssertCalled(t, "IssueCert", mock.Anything, tc.entityID, "backend", tc.ttl, tc.ipAddrs, certs.SubjectOptions{CommonName: tc.commonName}) |
| func (s *service) IssueCert(ctx context.Context, entityID, ttl string, ipAddrs []string, options SubjectOptions) (Certificate, error) { | ||
| // Set default common name if not provided | ||
| if options.CommonName == "" { | ||
| options.CommonName = entityID |
Contributor
There was a problem hiding this comment.
why would we assign an id to common name, this is wrong
Contributor
There was a problem hiding this comment.
common name should be validated as required
SammyOina
approved these changes
Sep 9, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
This is a feature addition for certs
What does this do?
Adds more features to certs.
Which issue(s) does this PR fix/relate to?
Have you included tests for your changes?
Did you document any new/modified features?
Notes