[ATB-150] Open API 3.0 Specs - TCP & UDP - Victor #70

Victorblsilveira · 2022-12-06T18:14:15Z

Pull request checklist

Documentation updated?

Affected functionality

No functionality really affected, only documentation API Specs

Description of change

Adding UDS and TCP OpenAPI 3.0 Specs

Additional Stuff
VS Code Plugin for OpenAPI 3.0 Specs Security Checks, visualization, and much more:

https://marketplace.visualstudio.com/items?itemName=42Crunch.vscode-openapi

pkg/server/openapi3/tcp_openapi3.yaml

pkg/server/openapi3/uds_openapi3.yaml

mchurichi · 2022-12-07T21:50:30Z

Just to make sure I got APIs and responsibilities right, I'll try to recap and throw a few questions/doubts that came to my mind:
We have 4 personas here: Galadriel Admin (a.k.a. super user or box admin), Tenant Admin, Harvester, and Harvester Admin.

Galadriel Admin
- is the one who onboards new tenants
- do we want to let him/her manage tenant resources? (CRUD operations over members, fed groups, etc)
Tenant Admin
- manages resources for a given tenant: onboards members, manage federation groups and requests memberships
- do we want to let him/her manage harvester membership requests? (accept/deny requests)
Harvester
- takes care of all bundle exchange related tasks: pushes it's colocated spire server bundle, and sets foreign bundles
Harvester Admin
- is the one who -with the help of the Galadriel Admin- sets up the Harvester through the onboarding process
- accepts/denies memberships requests

Given this, I wonder how we want to split our APIs. An API per persona? an API per transport protocol? We need to keep in mind that different methods (potentially in the same "API") may have different authorization mechanisms

Victorblsilveira · 2022-12-08T13:02:57Z

Just to make sure I got APIs and responsibilities right, I'll try to recap and throw a few questions/doubts that came to my mind: We have 4 personas here: Galadriel Admin (a.k.a. super user or box admin), Tenant Admin, Harvester, and Harvester Admin.

Galadriel Admin

is the one who onboards new tenants

do we want to let him/her manage tenant resources? (CRUD operations over members, fed groups, etc)

Tenant Admin

manages resources for a given tenant: onboards members, manage federation groups and requests memberships

do we want to let him/her manage harvester membership requests? (accept/deny requests)

Harvester

takes care of all bundle exchange related tasks: pushes it's colocated spire server bundle, and sets foreign bundles

Harvester Admin

is the one who -with the help of the Galadriel Admin- sets up the Harvester through the onboarding process

accepts/denies memberships requests

Given this, I wonder how we want to split our APIs. An API per persona? an API per transport protocol? We need to keep in mind that different methods (potentially in the same "API") may have different authorization mechanisms

I don't think that we should have APIs per persona since they can consume the same functionality. This is something that can be controlled through an authorization mechanism that we may consume. Initially, I would say to have one API for the TCP operations and another one for UDS. If we understand that is valid to split them ever further so we can do it, but for now I am not seeing a strong reason for that.

maxlambrecht

This is looking great, @Victorblsilveira !

I think we still need to put some thinking into which operations we need to accomplish the process of creating/registering a new Harvester. I left some thought about that and comments about other operations.

Do you plan to add the operation for adding a new tenant?

pkg/server/openapi3/harvester_openapi3.yaml

pkg/server/openapi3/management_openapi3.yaml

Victorblsilveira · 2022-12-16T19:48:14Z

This is looking great, @Victorblsilveira !

I think we still need to put some thinking into which operations we need to accomplish the process of creating/registering a new Harvester. I left some thought about that and comments about other operations.

Do you plan to add the operation for adding a new tenant?

Yes, I will add more endpoints to represent tenant operations. Tenant CRUD

wibarre

For the transparency log validation, we need to add the signed-trust-bundle and the cert of the public key generated by Galadriel server.

mchurichi · 2022-12-20T14:46:29Z

Just to summarize the thoughts I shared at yesterday's meeting: I think we need to be thoughtful on the permissions we give to admins. Allow admins to perform all CRUD operations over all resources just won't work, and it will cause more headaches than benefits (i.e. enable a tenant admin to update harvesters info could cause inconsistency issues, and even if we want to let them update some specific properties, we'd need to establish a granular permission system).

I emphasize the idea of continuing to focus on a clear definition of the personas, and the actions they would have to perform. This will help us think on the domain and the problem, and only then, drive the API design as a consequence.

pkg/server/openapi3/harvester_openapi3.yaml

pkg/server/openapi3/management_openapi3.yaml

pkg/server/openapi3/harvester.yaml

sonarqubecloud · 2023-01-20T14:48:24Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

jufantozzi

This is looking overall pretty good ⭐ great job @Victorblsilveira!

Most of comments are just minor fixes and trying to enforce consistency throughout the doc.

pkg/server/api/openapi3/harvester/harvester.yaml

pkg/server/api/openapi3/management/management.yaml

pkg/server/api/openapi3/harvester/harvester.yaml

maxlambrecht

Thanks for this work, @Victorblsilveira. I left several comments, I think the main thing to be re-worked is the onboard, how the join_token is sent, and also explicitly define the formats of the certificates and trust bundles.

pkg/server/api/openapi3/harvester/harvester.yaml

pkg/server/api/openapi3/management/management.yaml

maxlambrecht · 2023-03-06T19:12:48Z

pkg/server/api/openapi3/management/management.yaml

+      type: string
+      format: bytes
+      pattern: ^( *[A-Z0-9a-z] *.*)*$
+      maxLength: 400


Do we need to look more in detail at this maxLenght? SPIRE stores up to 16Mb size trust bundles (in blobs in MYSQL). This is from the models.go in SPIRE: Data []byte gorm:"size:16777215" // make MySQL to use MEDIUMBLOB (max 16MB) - doesn't affect PostgreSQL/SQLite

I was taking a look at the open API specification and maxLength is related to string length rather than size in bytes.
For restrictions in bytes size, we may need implementation reinforcement.

pkg/server/api/openapi3/management/management.yaml

maxlambrecht

Hey @Victorblsilveira, these are the entities that are currently defined in Galadriel with the latest changes in the data model, do you think we can have those entities defined in this spec using the same fields and names?

type TrustDomain struct {
	ID                uuid.NullUUID
	Name              spiffeid.TrustDomain `json:"name"`
	Description       string               `json:"description"`
	HarvesterSpiffeID spiffeid.ID          `json:"harvester_spiffe_id"`
	OnboardingBundle  []byte               `json:"onboarding_bundle"`
	CreatedAt         time.Time            `json:"created_at"`
	UpdatedAt         time.Time            `json:"updated_at"`
}

type Relationship struct {
	ID                  uuid.NullUUID
	TrustDomainAID      uuid.UUID            `json:"trust_domain_a_id"`
	TrustDomainBID      uuid.UUID            `json:"trust_domain_b_id"`
	TrustDomainAName    spiffeid.TrustDomain `json:"trust_domain_a_name"`
	TrustDomainBName    spiffeid.TrustDomain `json:"trust_domain_b_name"`
	TrustDomainAConsent bool                 `json:"trust_domain_a_consent"`
	TrustDomainBConsent bool                 `json:"trust_domain_b_consent"`
	CreatedAt           time.Time            `json:"created_at"`
	UpdatedAt           time.Time            `json:"updated_at"`
}

type JoinToken struct {
	ID              uuid.NullUUID
	Token           string
	Used            bool
	TrustDomainID   uuid.UUID            `json:"trust_domain_id"`
	TrustDomainName spiffeid.TrustDomain `json:"trust_domain_name"`
	ExpiresAt       time.Time            `json:"expires_at"`
	CreatedAt       time.Time            `json:"created_at"`
	UpdatedAt       time.Time            `json:"updated_at"`
}

// Bundle represents a SPIFFE Trust bundle along with its digest.
type Bundle struct {
	ID                 uuid.NullUUID
	Data               []byte               `json:"bundle"`
	Digest             []byte               `json:"bundle_digest"`
	Signature          []byte               `json:"signature"`
	DigestAlgorithm    string               `json:"digest_algorithm"`
	SignatureAlgorithm string               `json:"signature_algorithm"`
	SigningCert        []byte               `json:"signing_cert"`
	TrustDomainID      uuid.UUID            `json:"trust_domain_id"`
	TrustDomainName    spiffeid.TrustDomain `json:"trust_domain_name"`
	CreatedAt          time.Time            `json:"created_at"`
	UpdatedAt          time.Time            `json:"updated_at"`
}

Victorblsilveira · 2023-03-07T15:08:16Z

with the latest changes i

That is a great idea. Not sure if will be possible to have a 1:1 mapping in the open API spec due to some limitations in the specification but it is worth trying.

wibarre

Great job Victor. Thanks for working on this. I focused mainly in the management.yaml. The mechanics of the comms between harvester and server is being worked on, so I am waiting to hear what the outcome is from that front.

wibarre · 2023-03-07T20:56:09Z

pkg/server/api/openapi3/management/management.yaml

+      type: string
+      format: bytes
+      pattern: ^( *[A-Z0-9a-z] *.*)*$
+      maxLength: 400


Do we need to look more in detail at this maxLenght? SPIRE stores up to 16Mb size trust bundles (in blobs in MYSQL). This is from the models.go in SPIRE: Data []byte gorm:"size:16777215" // make MySQL to use MEDIUMBLOB (max 16MB) - doesn't affect PostgreSQL/SQLite

pkg/server/api/openapi3/management/management.yaml

pkg/server/api/openapi3/harvester/harvester.yaml

mchurichi · 2023-03-08T18:26:28Z

pkg/server/api/openapi3/harvester/harvester.yaml

+      type: string
+      format: byte
+      pattern: ^( *[A-Z0-9a-z] *.*)*$
+      maxLength: 65


64 bytes are enough to fit the most common hashing algorithms up to 512 bits (SHA-512, SHA3-512)

pkg/server/api/openapi3/harvester/harvester.yaml

mchurichi · 2023-03-08T20:29:29Z

pkg/server/api/openapi3/harvester/harvester.yaml

+    JWT:
+      type: string
+      format: uuid
+      maxLength: 4096 # Number of characters


where does this max length come from?

JWT specification does not have a max length limit. Although usually, JWT tokens are stored in session cookies or HTTP header who has limits around 4096 and 8192 respectively. We can leave it open if you see that does make sense to limit it.

+1 for using 8192

pkg/server/api/openapi3/management/management.yaml

pkg/server/api/openapi3/harvester/harvester.yaml

pkg/server/api/management/management.yaml

doc/API.md

Makefile

doc/API.md

pkg/common/api/schemas.cfg.yaml

pkg/common/api/schemas.yaml

Makefile

pkg/server/api/harvester/harvester.yaml

mchurichi · 2023-03-16T12:24:45Z

pkg/server/api/harvester/harvester.yaml

+        state:
+          type: array
+          items:
+            $ref: '#/components/schemas/TrustBundleSync'


could you please sync with @jufantozzi on what should be in both updates and state here?

I spoke with @jufantozzi (correct me if I am wrong) regarding this. The conclusion was, for now, the changes regarding if we are going to send the consent signature alongside the bundles, need a design discussion.

Ok, that makes sense, but still, and AFAIK updates and state shouldn't have the same structure.
state holds the digests of ALL the bundles a Harvester knows about.
updates holds all the NEW bundles a Harvester needs to set in SPIRE.

With @jufantozzi we worked on a flow breakdown doc a while ago, it may be a bit outdated though.

updates and state shouldn't have the same structure. state holds the digests of ALL the bundles a Harvester knows about. updates holds all the NEW bundles a Harvester needs to set in SPIRE.

This is correct.

The conclusion was, for now, the changes regarding if we are going to send the consent signature alongside the bundles, need a design discussion.

This is another discussion, in the new bundles (so in updates) each bundle will come with a consent, but this is still TBD.

We missed that the object for responses were the same, they shouldn't be. One maps trust-domains to digests, and the other maps trust-domains to bundles. I lead to this confusion because I missed this and thought you're talking about something else @mchurichi 😓 my bad

mchurichi

Just a suggestion for the sake of the review process: could you please wait until pushing the changes to mark a comment as resolved? Sometimes, I just don't know if a comment went stale or was discarded, and sometimes I doubt whether or not I made the suggestion I meant to 😆

pkg/common/api/schemas.yaml

wibarre

Thank you Victor for the great work.

pkg/server/api/admin/admin.yaml

pkg/server/api/harvester/harvester.yaml

mchurichi · 2023-03-21T20:25:42Z

doc/API.md

+    2.1 While in the [management](../pkg/server/api/management/) directory, run the following command:
+    - `oapi-codegen -config management.cfg.yaml management.yaml`


update to use admin instead of management

mchurichi · 2023-03-21T20:42:25Z

pkg/common/api/schemas.yaml

+      example: "trust.domain.com"
+    BundleDigest:
+      type: string
+      format: byte


Same thing as trust bundle format.

The hashing algorithms can output a short set of characters we can easily handle as simple strings. If we want to store it as bytes (I don't see a reason to do so) we should have them base-64-encoded, and the example above needs to be changed accordingly.

I'll just remove the format for now.

mchurichi · 2023-03-21T20:42:38Z

pkg/common/api/schemas.yaml

+        -----END CERTIFICATE-----
+    Signature:
+      type: string
+      format: byte


mchurichi · 2023-03-21T22:20:30Z

pkg/common/api/schemas.yaml

+        - onboarding_bundle
+        - harvester_spiffe_id


remove these, please

pkg/common/api/schemas.yaml

mchurichi · 2023-03-21T22:29:54Z

pkg/server/api/admin/admin.yaml

+        - name: status
+          in: query
+          schema:
+            type: string
+            enum: [approved, denied, pending]
+            default: pending


Are we allowing filtering by multiple statuses at the same time? it feels like we should.
Also, it would be more user-friendly to have a default value that returns all values rather than filtering the results at all.

mchurichi · 2023-03-22T11:21:24Z

pkg/server/api/admin/admin.yaml

+                type: array
+                items:
+                  $ref: '../../../common/api/schemas.yaml#/components/schemas/Relationship'
+                maxItems: 100


I'd suggest to remove this for now until we implement some sort of pagination.

pkg/common/api/schemas.yaml

Co-authored-by: Maximiliano Churichi <[email protected]> Signed-off-by: Victor Vieira Barros Leal da Silveira <[email protected]>

wibarre

Thank you Victor for retrofitting all the feedback into this PR.

sonarqubecloud · 2023-04-10T12:05:04Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

maxlambrecht

Excellent work @Victorblsilveira!

mchurichi

🙌

Victorblsilveira self-assigned this Dec 6, 2022

mgbcaio changed the title ~~Victor/atb 150~~ [DRAFT] Open API 3.0 Specs - TCP & UDP - Victor Dec 6, 2022

mchurichi reviewed Dec 7, 2022

View reviewed changes

maxlambrecht reviewed Dec 16, 2022

View reviewed changes

wibarre reviewed Dec 19, 2022

View reviewed changes

mchurichi reviewed Dec 20, 2022

View reviewed changes

mchurichi reviewed Jan 10, 2023

View reviewed changes

pkg/server/openapi3/harvester.yaml Outdated Show resolved Hide resolved

pkg/server/openapi3/harvester.yaml Outdated Show resolved Hide resolved

pkg/server/openapi3/harvester.yaml Outdated Show resolved Hide resolved

pkg/server/openapi3/harvester.yaml Outdated Show resolved Hide resolved

Victorblsilveira marked this pull request as ready for review January 12, 2023 13:01

Victorblsilveira requested review from jufantozzi and mgbcaio as code owners January 12, 2023 13:01

Victorblsilveira changed the title ~~[DRAFT] Open API 3.0 Specs - TCP & UDP - Victor~~ Open API 3.0 Specs - TCP & UDP - Victor Jan 13, 2023

Victorblsilveira changed the title ~~Open API 3.0 Specs - TCP & UDP - Victor~~ [ATB-150] Open API 3.0 Specs - TCP & UDP - Victor Jan 17, 2023

jufantozzi reviewed Mar 6, 2023

View reviewed changes

maxlambrecht reviewed Mar 6, 2023

View reviewed changes

pkg/server/api/openapi3/management/management.yaml Outdated Show resolved Hide resolved

maxlambrecht reviewed Mar 7, 2023

View reviewed changes

wibarre reviewed Mar 7, 2023

View reviewed changes

mchurichi reviewed Mar 8, 2023

View reviewed changes

mchurichi reviewed Mar 9, 2023

View reviewed changes

pkg/server/api/openapi3/harvester/harvester.yaml Outdated Show resolved Hide resolved

jufantozzi reviewed Mar 14, 2023

View reviewed changes

pkg/server/api/management/management.yaml Outdated Show resolved Hide resolved

mchurichi reviewed Mar 15, 2023

View reviewed changes

doc/API.md Outdated Show resolved Hide resolved

mchurichi reviewed Mar 16, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

mchurichi reviewed Mar 16, 2023

View reviewed changes

mchurichi reviewed Mar 17, 2023

View reviewed changes

mchurichi reviewed Mar 20, 2023

View reviewed changes

pkg/common/api/schemas.yaml Show resolved Hide resolved

wibarre reviewed Mar 21, 2023

View reviewed changes

Victor Vieira Barros Leal da Silveira and others added 2 commits March 21, 2023 17:29

feat: Galadriel Server OpenAPI 3.0 specifications

bf7bbb2

fix: Correcting query params

e0314e6

Victorblsilveira force-pushed the victor/atb-150 branch from a733f78 to e0314e6 Compare March 21, 2023 20:36

Victorblsilveira added 3 commits March 21, 2023 17:39

Merge remote-tracking branch 'origin/main' into victor/atb-150

94d43f0

fix: Adding empty line at the end

43a90f2

feat: Updating boilerplate

ec167f5

mchurichi reviewed Mar 21, 2023

View reviewed changes

fix: Correction in formats and names

45ed1ee

mchurichi reviewed Mar 22, 2023

View reviewed changes

pkg/common/api/schemas.yaml Outdated Show resolved Hide resolved

Victorblsilveira and others added 5 commits March 22, 2023 11:36

Update pkg/common/api/schemas.yaml

1321a0b

Co-authored-by: Maximiliano Churichi <[email protected]> Signed-off-by: Victor Vieira Barros Leal da Silveira <[email protected]>

fix: Removing maxItems and some required fields

45ec203

feat: Regenerating boilerplate

a1aa8b3

Merge branch 'main' into victor/atb-150

df2ffbf

Merge branch 'main' into victor/atb-150

0bdf07b

wibarre previously approved these changes Apr 7, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into victor/atb-150

195d9c0

Victorblsilveira dismissed wibarre’s stale review via 195d9c0 April 10, 2023 12:02

Merge branch 'main' into victor/atb-150

af721a4

maxlambrecht approved these changes Apr 10, 2023

View reviewed changes

mchurichi approved these changes Apr 10, 2023

View reviewed changes

Victorblsilveira merged commit 76e0395 into main Apr 10, 2023

Victorblsilveira deleted the victor/atb-150 branch April 10, 2023 14:40

		2.1 While in the [management](../pkg/server/api/management/) directory, run the following command:
		- `oapi-codegen -config management.cfg.yaml management.yaml`

[ATB-150] Open API 3.0 Specs - TCP & UDP - Victor #70

[ATB-150] Open API 3.0 Specs - TCP & UDP - Victor #70

Conversation

Victorblsilveira commented Dec 6, 2022 • edited Loading

mchurichi commented Dec 7, 2022

Victorblsilveira commented Dec 8, 2022

maxlambrecht left a comment

Choose a reason for hiding this comment

Victorblsilveira commented Dec 16, 2022

wibarre left a comment

Choose a reason for hiding this comment

mchurichi commented Dec 20, 2022

sonarqubecloud bot commented Jan 20, 2023

jufantozzi left a comment

Choose a reason for hiding this comment

maxlambrecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxlambrecht left a comment

Choose a reason for hiding this comment

Victorblsilveira commented Mar 7, 2023

wibarre left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jufantozzi Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

mchurichi left a comment

Choose a reason for hiding this comment

wibarre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wibarre left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 10, 2023

maxlambrecht left a comment

Choose a reason for hiding this comment

mchurichi left a comment

Choose a reason for hiding this comment

Victorblsilveira commented Dec 6, 2022 •

edited

Loading

wibarre left a comment •

edited

Loading

jufantozzi Mar 21, 2023 •

edited

Loading