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 and extend map server #48

Merged
merged 16 commits into from
Mar 11, 2024
Merged

Fix and extend map server #48

merged 16 commits into from
Mar 11, 2024

Conversation

cyrill-k
Copy link
Collaborator

@cyrill-k cyrill-k commented Jan 29, 2024

Various Improvements and fixes for the map server:

  • fix MHT root value bug (only allow single root value at each point in time)
  • fix SPCT signature calculation (now includes timestamp in signature calculation)
  • allow fetching certificates and policies with a single API call
  • add option to insert single policy from file
  • add helper option to verify policy chain constraints (used by browser extension's web assembly part)
  • add documentation

This change is Reviewable

Copy link
Member

@juagargi juagargi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 22 of 22 files at r1, 2 of 5 files at r2.
Reviewable status: 21 of 24 files reviewed, 3 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained


cmd/mapserver/main.go line 58 at r1 (raw file):

			err = run(*updateVar)
		}
	}
	switch{
	case *createSampleConfig:
		err = writeSampleConfig()
	case *insertPolicyVar != "":
		err = insertPolicyFromFile(*insertPolicyVar)
	default:
		err = run(*updateVar)
	}

Code quote:

		if *insertPolicyVar != "" {
			err = insertPolicyFromFile(*insertPolicyVar)
		} else {
			err = run(*updateVar)
		}
	}

pkg/db/mysql/common.go line 11 at r1 (raw file):

// RetrievePolicyPayloads returns the payload for each certificate OR policy identified by the IDs
// parameter, in the same order (element i corresponds to IDs[i]).
func (c *mysqlDB) RetrieveCertificateOrPolicyPayloads(ctx context.Context, IDs []*common.SHA256Output,

Maybe we should have a UT checking this function.


pkg/mapserver/updater/updater.go line 292 at r1 (raw file):

	policySubjects := make([]string, len(policies))
	for i, pol := range policies {
		payloads[i] = pol.Raw()

This change makes me think that you found a bug here. Is it related to your comment in the json functionality about not refreshing the Raw bytes?
If there was a bug here, can we add a UT to ensure it won't happen again? Or do we need something more sophisticated?


tools/create_schema.sh line 137 at r1 (raw file):

    -- constraints to ensure that only a single root value exists at any time by having a single possible value for the primary key
    single_row_pk char(25) NOT NULL PRIMARY KEY DEFAULT 'PK_RestrictToOneRootValue' CHECK (single_row_pk='PK_RestrictToOneRootValue')

I had to ask ChatGPT to explain this to me 😛 . TIL how to restrict a table to one row.

Copy link
Member

@juagargi juagargi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

Copy link
Collaborator Author

@cyrill-k cyrill-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 14 of 22 files at r1, 3 of 5 files at r2, 11 of 11 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained


cmd/mapserver/main.go line 58 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…
	switch{
	case *createSampleConfig:
		err = writeSampleConfig()
	case *insertPolicyVar != "":
		err = insertPolicyFromFile(*insertPolicyVar)
	default:
		err = run(*updateVar)
	}

Done


pkg/db/mysql/common.go line 11 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Maybe we should have a UT checking this function.

Done


pkg/mapserver/updater/updater.go line 292 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

This change makes me think that you found a bug here. Is it related to your comment in the json functionality about not refreshing the Raw bytes?
If there was a bug here, can we add a UT to ensure it won't happen again? Or do we need something more sophisticated?

I think the problem is that if an object is created programmatically (i.e., NewPolicyCertificate()), the JSONField attribute was not set. I changed the function to either return the JSONField attribute if it is set or otherwise marshal the object into JSON and return it. The only potential issue I see with this solution is that if the JSONField is already set and one modifies an attribute of an object, the JSONField and the object will not contain the same information anymore.
One solution would be to simply always do the marshalling and remove the JSONField, but this might have an impact on the performance.

I also added a UT for this.

What do you think? Can we somehow make all attributes immutable to prevent this?

Also, if we keep the current solution, we should probably (re-)set the JSONField once Raw() is called.


tools/create_schema.sh line 137 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

I had to ask ChatGPT to explain this to me 😛 . TIL how to restrict a table to one row.

I did it old school and asked google :D

Copy link
Member

@juagargi juagargi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 11 of 11 files at r3.
Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained


pkg/mapserver/updater/updater.go line 292 at r1 (raw file):

Previously, cyrill-k wrote…

I think the problem is that if an object is created programmatically (i.e., NewPolicyCertificate()), the JSONField attribute was not set. I changed the function to either return the JSONField attribute if it is set or otherwise marshal the object into JSON and return it. The only potential issue I see with this solution is that if the JSONField is already set and one modifies an attribute of an object, the JSONField and the object will not contain the same information anymore.
One solution would be to simply always do the marshalling and remove the JSONField, but this might have an impact on the performance.

I also added a UT for this.

What do you think? Can we somehow make all attributes immutable to prevent this?

Also, if we keep the current solution, we should probably (re-)set the JSONField once Raw() is called.

Yes, I've just created issue #51 to cover this.

@juagargi juagargi merged commit 5b4a44e into master Mar 11, 2024
1 check passed
@juagargi juagargi deleted the cyrill-wip branch March 11, 2024 15:51
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.

2 participants