lahirujayathilake commented on code in PR #532:
URL: https://github.com/apache/airavata-custos/pull/532#discussion_r3819100758
##########
extensions/SSH-Certificate-Signer/internal/server/routes.go:
##########
@@ -78,6 +82,13 @@ func NewRouter(
r.Get("/api/v1/certificates", handlers.Certificates)
r.Get("/api/v1/certificates/{serial}",
handlers.CertificateDetail)
r.Get("/api/v1/userinfo", handlers.UserInfo)
+
+ r.With(CorePrivilegeMiddleware(coreAuthorizer,
auth.SignerCertificatesRead)).
+ Get("/api/v1/admin/certificates",
handlers.AdminCertificates)
+ r.With(CorePrivilegeMiddleware(coreAuthorizer,
auth.SignerCertificatesRead)).
+ Get("/api/v1/admin/certificates/{serial}",
handlers.AdminCertificate)
+ r.With(CorePrivilegeMiddleware(coreAuthorizer,
auth.SignerCertificatesWrite)).
Review Comment:
User should be able to revoke their own certificates.
##########
extensions/SSH-Certificate-Signer/internal/store/certificate_query.go:
##########
@@ -133,6 +140,94 @@ func (d *DB) ListCertificatesByEmail(ctx context.Context,
email string, limit, o
}, nil
}
+// ListCertificates returns a deployment-wide page for privileged
administrators.
+func (d *DB) ListCertificates(ctx context.Context, limit, offset int)
(*CertificateListResult, error) {
+ if limit <= 0 {
+ limit = 20
+ }
+ if limit > 100 {
+ limit = 100
+ }
+ if offset < 0 {
+ offset = 0
+ }
+
+ var total int
+ if err := d.QueryRowContext(ctx, `SELECT COUNT(*) FROM
certificate_issuance_logs`).Scan(&total); err != nil {
Review Comment:
This counts every row in the table on every page load, and
`certificate_issuance_logs` only grows. If the dashboard doesn't need an exact
total, drop the count and page on `issued_at` instead of OFFSET.
##########
extensions/SSH-Certificate-Signer/internal/auth/core_authorizer.go:
##########
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0.
Review Comment:
Use the correct license with the formatting. Refer -
https://github.com/apache/airavata-custos/blob/f31132c5990055b07d1b561ab9cc4506f05ee05e/cmd/server/main.go#L1
Update the rest as well.
##########
extensions/SSH-Certificate-Signer/internal/store/revocation.go:
##########
@@ -56,3 +75,119 @@ func (d *DB) InsertRevocationEvent(ctx context.Context, ev
*RevocationEvent) err
}
return nil
}
+
+// RevokeCertificateBySerial revokes the certificate identified by serial. It
does
+// NOT scope by owner — authorization is enforced by the caller (an
administrator
+// holding signer:certificates:write, or a trusted machine client). It is
+// idempotent: a repeat revoke returns the existing revocation
(AlreadyRevoked=true)
+// without inserting a duplicate event. The revocation event inherits the
+// certificate's tenant/client.
+func (d *DB) RevokeCertificateBySerial(
+ ctx context.Context,
+ serialNumber int64,
+ reason string,
+ revokedBy string,
+) (*RevokedCertificate, error) {
+ return d.revokeCertificateBySerial(ctx, serialNumber, reason,
revokedBy, false)
+}
+
+// RevokeActiveCertificateBySerial is the administrator-facing variant. It
+// refuses a first-time revocation outside the certificate validity interval,
+// while preserving idempotent success for retries of an earlier revocation.
+func (d *DB) RevokeActiveCertificateBySerial(
+ ctx context.Context,
+ serialNumber int64,
+ reason string,
+ revokedBy string,
+) (*RevokedCertificate, error) {
+ return d.revokeCertificateBySerial(ctx, serialNumber, reason,
revokedBy, true)
+}
+
+func (d *DB) revokeCertificateBySerial(
Review Comment:
Shouldn't include logic in a store class. Checking if the certificate is
expired and deciding to return the existing revocation are business rules, not
storage.
Move them to a service. The store should only read the certificate, read the
revocation and insert one. The service holds the transaction.
##########
extensions/SSH-Certificate-Signer/internal/handler/admin_certificates.go:
##########
Review Comment:
Remove this class and use the existing certificate handler. Revoking is the
same regardless of the user type. A user can revoke their own and a privileged
user can revoke any.
##########
pkg/models/privilege.go:
##########
Review Comment:
Drop these from here. The `core` shouldn't have extension privileges. We can
use Next.js multi-zones for extension UIs, so the signer serves and gates its
own admin pages under /signer. `core` doesn't need to know these keys.
Load them from config instead
```
privileges:
extensions:
- signer:certificates:read
- signer:certificates:write
```
Register in cmd/server/main.go after LoadConfig, before connectors load.
Reject anything starting with `core:` so config can't inject a core privilege.
##########
extensions/SSH-Certificate-Signer/internal/store/revocation.go:
##########
Review Comment:
How do you handle the KRL binary updates?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
