[DPE-5306][SECURITY] No write before protocol init#194
Merged
Conversation
juditnovak
force-pushed
the
DPE-5306/no_write_before_protocol_init
branch
2 times, most recently
from
11d633e to
0ceadda
Compare
juditnovak
force-pushed
the
DPE-5306/no_write_before_protocol_init
branch
from
0ceadda to
40e2d13
Compare
juditnovak
commented
Sep 9, 2024
| """Set values for fields not caring whether it's a secret or not.""" | ||
| req_secret_fields = [] | ||
|
|
||
| if self.fetch_relation_field(relation.id, self.DATABASE_FIELD) is None: |
Contributor
Author
There was a problem hiding this comment.
Note: As for current conventions, the field MUST exist, but may be empty.
https://github.com/canonical/postgresql-test-app/blob/main/src/charm.py#L155
delgod
approved these changes
Sep 9, 2024
juditnovak
force-pushed
the
DPE-5306/no_write_before_protocol_init
branch
from
40e2d13 to
5ed6b51
Compare
juditnovak
force-pushed
the
DPE-5306/no_write_before_protocol_init
branch
from
084987f to
a95b6f5
Compare
juditnovak
force-pushed
the
DPE-5306/no_write_before_protocol_init
branch
from
a95b6f5 to
a46672c
Compare
delgod
approved these changes
Sep 24, 2024
welpaolo
approved these changes
Sep 26, 2024
| class ProviderData(Data): | ||
| """Base provides-side of the data products relation.""" | ||
|
|
||
| DATABASE_FIELD = "database" |
Contributor
There was a problem hiding this comment.
DATABASE_FIELD is a little bit misleading maybe we can find a better name such as RESOURCE_FIELD
marceloneppel
approved these changes
Sep 26, 2024
tonyandrewmeyer
pushed a commit
to tonyandrewmeyer/data-platform-libs
that referenced
this pull request
Jan 28, 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 join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Issue
#193
In case any write operation may take place before the protocol is established the
Providermay not yet be aware whether secrets are to be used or not. Thus, the write operation may end up as plain text in the databag, instead of secrets.Example:
A charm may receive TLS certs anytime, that are to be shared on a client relation.
When the charm got the TLS secrest, it just "blindly" writes them using
update_relation_data()onupdate-statusorcertificate-availableor so.Which technically could be executed BEFORE the
Requirer(!!!!)relation-createdhandler would put therequested-secretslist -- highlighting that TLS certs are stored as a Juju Secret on this relation.Solution
No
Providerwrite operation is allowed before the initial part of the protocol is finished.This can be assured by waiting until the
Requirerwould add thedatabasefield to the Relation Data. By this time theRequirermust have executed itsrelation-createdhook (whererequested-secretsis written to the databag)NOTE: Given that the change is straightforward, while the scenario is non-trivial to reproduce. I believe that unittest coverage is sufficient.
NOTE2: This may be a breaking change for charms that were publishing sensitive data premature. Clearly corresponding charm code is to be fixed.
See PG corresponding fix with healthy pipelines: canonical/postgresql-operator#615