Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
assertions: add "repair" assertion #3211
Conversation
mvo5
added some commits
Apr 18, 2017
All checks have failed
mvo5
requested a review
from
jdstrand
Apr 19, 2017
| + assertionBase | ||
| + | ||
| + series []string | ||
| + models []string |
niemeyer
Apr 25, 2017
Contributor
Maybe.. I'm not entirely sure, but it's easier to implement support and decide later whether that's a good idea than the opposite.
| + | ||
| +// Models returns the models that this assertion is valid for. | ||
| +func (em *Repair) Models() []string { | ||
| + return em.models |
zyga
Apr 19, 2017
Contributor
If you go ahead with a wildcard support to models we should have a function that checks if a model needs a given repair.
| +// code to fixup broken systems. It can be limited by series and models. | ||
| +type Repair struct { | ||
| + assertionBase | ||
| + |
zyga
Apr 19, 2017
Contributor
I'd also, perhaps, include a field that says which other repairs must be first applied for this repair to make sense. I'd say we may different strategy to fix issues if another repair was already applied. Those could be kept in state.
niemeyer
Apr 25, 2017
Contributor
Hopefully we can make this a bit simpler. See points around the repair ID.
zyga
Apr 26, 2017
Contributor
Aha, indeed, that is even better. I like the deterministic nature of such approach.
mvo5
added some commits
Apr 20, 2017
All checks have failed
All checks have failed
Some checks were not successful
All checks have passed
All checks have passed
| @@ -58,6 +58,7 @@ func (at *AssertionType) MaxSupportedFormat() int { | ||
| var ( | ||
| AccountType = &AssertionType{"account", []string{"account-id"}, assembleAccount, 0} | ||
| AccountKeyType = &AssertionType{"account-key", []string{"public-key-sha3-384"}, assembleAccountKey, 0} | ||
| + RepairType = &AssertionType{"repair", []string{"repair-id"}, assembleRepair, 0} |
pedronis
Apr 24, 2017
Contributor
we need to decide what's really the primary key/how we will use it, especially in light of the idea of opening this further, that's the thing that is hardest to change after the fact
mvo5
Apr 25, 2017
Collaborator
I updated the branch now to have (brand-id, repair-id) as the primary key. even if we don't open this up to brands it should not do any harm and make things easier for the future.
This was referenced Apr 24, 2017
niemeyer
approved these changes
Apr 25, 2017
Some comments for your consideration. LGTM once these are handled to your satisfaction, but please make sure to get a
| +// code to fixup broken systems. It can be limited by series and models. | ||
| +type Repair struct { | ||
| + assertionBase | ||
| + |
zyga
Apr 19, 2017
Contributor
I'd also, perhaps, include a field that says which other repairs must be first applied for this repair to make sense. I'd say we may different strategy to fix issues if another repair was already applied. Those could be kept in state.
niemeyer
Apr 25, 2017
Contributor
Hopefully we can make this a bit simpler. See points around the repair ID.
zyga
Apr 26, 2017
Contributor
Aha, indeed, that is even better. I like the deterministic nature of such approach.
| + assertionBase | ||
| + | ||
| + series []string | ||
| + models []string |
niemeyer
Apr 25, 2017
Contributor
Maybe.. I'm not entirely sure, but it's easier to implement support and decide later whether that's a good idea than the opposite.
| +// that follows a convention like "REPAIR-123". Similar to a CVE there | ||
| +// should be a public place to look up details about the repair-id | ||
| +// (e.g. the snapcraft forum). | ||
| +func (em *Repair) RepairID() string { |
niemeyer
Apr 25, 2017
Contributor
We can either have a single ID, or a split one ("REPAIR", 123). Having a single one is probably a way to reinforce the idea of such IDs, so seems nice, and either way we'll end up with that pair as the primary key, so not much different.
If keeping it as a single string, we should probably make sure it conforms to the format <key>-<num> though, so that we can take some further decisions based on that. One important behavior to establish is ordering. We probably don't want arbitrary ordering, but at the same time we probably don't want unrelated patch sets to depend on each other. Instead, we should probably sequence entries with the same <key> to ensure the order of <num> is respected, applying one after another, while accepting independent keys to be applied out of order.
Comments? Ideas?
zyga
Apr 26, 2017
Contributor
To Gustavo's point I think that we may see two classes of repair assertions. Those that are issued by a particular brand to address an isolated issue (where isolated is broad enough to include all devices from that brand). The other kind may be issued by Canonical to fix all core systems (or at least check if all core systems require any kind of fix). I'd say that the order should be arranged such that all canonical assertions are applied before any other assertions are considered.
What do you think?
pedronis
Apr 26, 2017
•
Edited 1 time
-
pedronis
Apr 26, 2017
Contributor
-
Apr 26, 2017
I see two approaches either
-
we have a clever service that gives us back all relevant repairs,
-
or we say that devices are sort of subscribed to sequences of repairs, all would be subscribed to (canonical, repair-#) at least, than we would fetch the next (increased seq num for each of them)
a cons of 1 is that we need to think exactly how to not return a huge list, what kind of information to send to let the other side do filtering for us, the more complex information we need to send the more it's fragile because we might not be able to find the information exactly because we are broken, another cons is that the service needs to be built (even if we open up the querying capabilities of the assertion service I don't think they fit this task)
a cons of 2 is that it seems to need some integration into the process of image building/configuration, we need to list streams at that point somehow, because ideally we want to both give a stream and a starting point (reapairs older than that are known to be not relevant), we can specify that when we build new snapd/core for the default streams but not for the per device/brand ones
| +PAYLOAD: | ||
| +H4sIAJJt+FgAA+3STQrCMBDF8ax7ihEP0CkxyXn8iCZQE2jr/W11Iwi6KiL8f5u3mLd4i0mx76tZ | ||
| +l86Cc0t2welrPu2c6awGr95bG4x26rw1oivveriN034QMfFSy6fet/uf2m7aQy7tmJp4TFXS8g5y | ||
| +HupVphQllzGfYvPrkQAAAAAAAAAAAAAAAACAN3dTp9TNACgAAA== |
niemeyer
Apr 25, 2017
Contributor
I just had an 80s flashback here.. with uuencode, usenet, ... Thanks for the test, and the mind trip! :)
zyga
Apr 26, 2017
Contributor
I cannot wait to see that payload that uses shell, python and perl together.
Some checks were not successful
zyga
reviewed
Apr 26, 2017
Looks good.
My only comment that stands is the one where I feel we should do globs for model matching and then perhaps offer a function for matching this.
| +// code to fixup broken systems. It can be limited by series and models. | ||
| +type Repair struct { | ||
| + assertionBase | ||
| + |
zyga
Apr 19, 2017
Contributor
I'd also, perhaps, include a field that says which other repairs must be first applied for this repair to make sense. I'd say we may different strategy to fix issues if another repair was already applied. Those could be kept in state.
niemeyer
Apr 25, 2017
Contributor
Hopefully we can make this a bit simpler. See points around the repair ID.
zyga
Apr 26, 2017
Contributor
Aha, indeed, that is even better. I like the deterministic nature of such approach.
| +// that follows a convention like "REPAIR-123". Similar to a CVE there | ||
| +// should be a public place to look up details about the repair-id | ||
| +// (e.g. the snapcraft forum). | ||
| +func (em *Repair) RepairID() string { |
niemeyer
Apr 25, 2017
Contributor
We can either have a single ID, or a split one ("REPAIR", 123). Having a single one is probably a way to reinforce the idea of such IDs, so seems nice, and either way we'll end up with that pair as the primary key, so not much different.
If keeping it as a single string, we should probably make sure it conforms to the format <key>-<num> though, so that we can take some further decisions based on that. One important behavior to establish is ordering. We probably don't want arbitrary ordering, but at the same time we probably don't want unrelated patch sets to depend on each other. Instead, we should probably sequence entries with the same <key> to ensure the order of <num> is respected, applying one after another, while accepting independent keys to be applied out of order.
Comments? Ideas?
zyga
Apr 26, 2017
Contributor
To Gustavo's point I think that we may see two classes of repair assertions. Those that are issued by a particular brand to address an isolated issue (where isolated is broad enough to include all devices from that brand). The other kind may be issued by Canonical to fix all core systems (or at least check if all core systems require any kind of fix). I'd say that the order should be arranged such that all canonical assertions are applied before any other assertions are considered.
What do you think?
pedronis
Apr 26, 2017
•
Edited 1 time
-
pedronis
Apr 26, 2017
Contributor
-
Apr 26, 2017
I see two approaches either
-
we have a clever service that gives us back all relevant repairs,
-
or we say that devices are sort of subscribed to sequences of repairs, all would be subscribed to (canonical, repair-#) at least, than we would fetch the next (increased seq num for each of them)
a cons of 1 is that we need to think exactly how to not return a huge list, what kind of information to send to let the other side do filtering for us, the more complex information we need to send the more it's fragile because we might not be able to find the information exactly because we are broken, another cons is that the service needs to be built (even if we open up the querying capabilities of the assertion service I don't think they fit this task)
a cons of 2 is that it seems to need some integration into the process of image building/configuration, we need to list streams at that point somehow, because ideally we want to both give a stream and a starting point (reapairs older than that are known to be not relevant), we can specify that when we build new snapd/core for the default streams but not for the per device/brand ones
| +} | ||
| + | ||
| +// sanity | ||
| +var _ consistencyChecker = (*Repair)(nil) |
| +// sanity | ||
| +var _ consistencyChecker = (*Repair)(nil) | ||
| + | ||
| +var validRepairID = regexp.MustCompile("^[a-z]+-[0-9]+") |
| +PAYLOAD: | ||
| +H4sIAJJt+FgAA+3STQrCMBDF8ax7ihEP0CkxyXn8iCZQE2jr/W11Iwi6KiL8f5u3mLd4i0mx76tZ | ||
| +l86Cc0t2welrPu2c6awGr95bG4x26rw1oivveriN034QMfFSy6fet/uf2m7aQy7tmJp4TFXS8g5y | ||
| +HupVphQllzGfYvPrkQAAAAAAAAAAAAAAAACAN3dTp9TNACgAAA== |
niemeyer
Apr 25, 2017
Contributor
I just had an 80s flashback here.. with uuencode, usenet, ... Thanks for the test, and the mind trip! :)
zyga
Apr 26, 2017
Contributor
I cannot wait to see that payload that uses shell, python and perl together.
| + "repair-id: repair-42\n"+ | ||
| + "series:\n"+ | ||
| + " - 16\n"+ | ||
| + "MODELSLINE\n"+ |
mvo5
May 10, 2017
Collaborator
Mostly done it this way out of consistency with the existing tests in asserts/*
| +type Repair struct { | ||
| + assertionBase | ||
| + | ||
| + series []string |
pedronis
Apr 26, 2017
Contributor
as I wondered I think on the forum, should we add a filter for architecures as well?
| +} | ||
| + | ||
| +// BrandID returns the brand identifier that signed this assertion. | ||
| +func (em *Repair) BrandID() string { |
| + | ||
| +// Implement further consistency checks. | ||
| +func (em *Repair) checkConsistency(db RODatabase, acck *AccountKey) error { | ||
| + if !db.IsTrustedAccount(em.AuthorityID()) { |
pedronis
Apr 26, 2017
Contributor
I suppose if we open it up we don't want this check, though we probably want to make sure that brand-id and authority-id match (we have checks like that for other assertions) in assemble
also atm this one bit is not tested
|
marking this blocked because from the forum post it seems there are still some open questions around the primary key of this, that hopefully will be solved at the sprint next week |
pedronis
added
the
Blocked
label
Apr 28, 2017
mvo5
added some commits
May 10, 2017
Some checks were not successful
Some checks were not successful
mvo5
removed
the
Blocked
label
May 10, 2017
|
Removing the "blocked" label, we are hopefully unblocked after the discussion at the sprint. This should be close to what we discussed at the sprint now. |
All checks have passed
| +// the repair-id can either be: | ||
| +// - repair-$ID | ||
| +// - $brand-$ID | ||
| +// - $brand-$model-$ID |
pedronis
May 11, 2017
Contributor
mmh, brand itself can contain '-', wondering if we need $brand_$model-$ID or $brand:$model-$ID
pedronis
May 12, 2017
Contributor
thanks, we might revisit this again when we actually implement things with the store, given that brand-id is also part of the primary key, is not super clear it needs to be repeated in the id on the other hand it feels good that the id is for practical purposes a full handle to the assertion
mvo5
added some commits
May 12, 2017
All checks have passed
| +// the repair-id can either be: | ||
| +// - repair-$ID | ||
| +// - $brand-$ID | ||
| +// - $brand-$model-$ID |
pedronis
May 11, 2017
Contributor
mmh, brand itself can contain '-', wondering if we need $brand_$model-$ID or $brand:$model-$ID
pedronis
May 12, 2017
Contributor
thanks, we might revisit this again when we actually implement things with the store, given that brand-id is also part of the primary key, is not super clear it needs to be repeated in the id on the other hand it feels good that the id is for practical purposes a full handle to the assertion
| +func assembleRepair(assert assertionBase) (Assertion, error) { | ||
| + err := checkAuthorityMatchesBrand(&assert) | ||
| + if err != nil { | ||
| + return nil, err |
pedronis
approved these changes
May 12, 2017
missing a test I think but looks good as starting point to merge, we can refine on the way to actually use this
mvo5
added some commits
May 12, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
mvo5 commentedApr 19, 2017
•
Edited 1 time
-
mvo5
Apr 19, 2017
This is the boilerplate code for the "repair" assertion. Some background is available at https://forum.snapcraft.io/t/repair-capability-emergency-fixes/311
It still contains the "since/until" headers mostly because in a previous discussion the security team suggested them. I add Jamie as a reviewer so he can chime in :)