[Amendment] Disallow setting an account's regular key to match its master key #2873
Conversation
|
Maybe something like "fixUniqueMasterKey"? It is more or less an edge case and likely unintended, so I'd keep the "fix" part in front (compared to pure feature amendments like SHAmap_v2). Alternatively maybe "fixRegularNeqMasterKey". |
Jenkins Build SummaryBuilt from this commit Built at 20190415 - 17:30:10 Test Results
|
src/test/app/SetRegularKey_test.cpp
Outdated
| auto jv = regkey(alice, bob); | ||
| jv[sfFlags.fieldName] = tfUniversalMask; | ||
| env(jv, ter(temINVALID_FLAG)); | ||
| } |
There was a problem hiding this comment.
Why did you roll the separate test functions into run? Having the functions divided up helps with readability and maintainability.
There was a problem hiding this comment.
I disagree. I came into this class thinking adding a method would add a test. No, you must add a call to that method. Adding a method does not give the test a name either, though you have to give the method a name. Methods are not run independently, with separate setup and teardown. This class tries to look like other test frameworks, but it falls to live up to the expectations set by those frameworks. Instead, I'd rather it look exactly like what it's doing, so that adding to it is straightforward and intuitive.
There was a problem hiding this comment.
I have side with Ed on this one. I hear that you have expectations based on other test frameworks that you've used. And, believe me, I sympathize that the framework we're using could be better. But there are a ton of tests already in this code base that follow the model that you just unrolled. I believe, at least, the tests within this code base should be self consistent.
If you want to argue that all of the tests in the code base should be unrolled you're welcome to make that argument to the team and provide a plan for making all the tests consistent with your vision. If the team agrees with your vision then charge ahead. But flattening just one test only adds to confusion.
Just my opinion.
There was a problem hiding this comment.
The tests are already inconsistent. Here are some that have a fully unrolled run() matching the pattern I use here: Buffer_test, TaggedCache_test, Slice_test, Scheduler_test, LedgerReplay_test. One surprising case is Manifest_test, which has a mix of the two patterns. Given that both patterns are in common use, I'd like to tip the scales in favor of the unroll. I'll hold a quick poll in chat to see which way the winds blow, and if I'm in the minority, I'll revert the unroll, but I don't think a plan beyond "unroll them as I touch them" should be required given the existing inconsistency.
There was a problem hiding this comment.
Well, I've got quite the uphill battle ahead of me. I've reverted the unroll for now... soon.jpg
There was a problem hiding this comment.
We are inconsistent: some tests are in methods called from run and some are in run directly. I don't think we should settle on a single "hard" rule because such rules force us to do the wrong thing.
Consider that we have several tests that are several hundred lines long. Put all those in a single function and you'd end up with a function that's several thousand lines and that's just a recipe for disaster. Grouping a single set of "tests" inside a function just makes for easier to read code.
But then again, we have tests that are less than 10 lines long. Do we want a separate function for those? Probably not because it seems like an unnecessary level of indirection.
I think we need to exercise our judgement, and decide on a case-by-case basis. Yes, that means some inconsistency between files; Things like Manifest_test should probably be cleaned up to be one way or the other.
One thought might be to make testcase take both a name and a lambda. That way, we could develop a common style:
void run()
{
testcase("Try one thing", []()
{
});
testcase("Try a second thing", []()
{
});
for (int i = 0; i != 8; ++i)
{
testcase ("Try various things: " + std::to_string(i), []()
{
}(i));
}
}
At least until we migrate to a different testing framework and away from test_suite; then we can see if one particular style is better suited.
src/ripple/protocol/TER.h
Outdated
| @@ -96,6 +96,7 @@ enum TEMcodes : TERUnderlyingType | |||
| temBAD_SIGNATURE, | |||
| temBAD_SRC_ACCOUNT, | |||
| temBAD_TRANSFER_RATE, | |||
| temCANT_USE_MASTER_KEY, | |||
There was a problem hiding this comment.
I'm not super thrilled with this name. The existing temBAD_SIGNER may be perfectly appropriate, and if not I suggest something like temBAD_REGKEY.
|
Should we also change the behavior for any existing accounts whose master key matches their regular key? Perhaps in the code where we reject a transaction because it's signed with the master key and the master key was disabled, check if the regular key and master keys are identical and, if so, allow the transaction to execute. I realize this changes the behavior of existing accounts, but I doubt anyone intentionally rendered an account unusable by setting the regular key to the master key and disabling the master key. We have sane ways to render an account unusable (set regular key to provably unusable value and disable master key). |
|
How many accounts are currently affected by this anyways? |
|
We've discussed reversing the checks for "does the signing key match the master key and the master key is not disabled" and "does the signing key match the regular key", which would unlock currently locked accounts (there are only around 20). That could go in this change (I think it requires an amendment anyway). |
|
|
||
| if (pkAccount == id) | ||
| // Check that the signing key is authorized. |
There was a problem hiding this comment.
This code has, suddenly, gotten quite convoluted. May I suggest the following alternative?
if (ctx.view.rules().enabled(fixDisabledRegularKey))
{
if (hasRegularKey && sleAccount->getAccountID(sfRegularKey) == idSigner)
{
// The regular key was used to sign:
return tesSUCCESS;
}
if (!sleAccount->isFlag(lsfDisableMaster) && idSigner == idAccount)
{
// The master key was used to sign:
return tesSUCCESS;
}
// Signing key does not match master or regular key.
return tefBAD_AUTH;
}
else
{
// original code here
}
This, obviously, is a more involved change, since it changes the error codes we return so several unit tests will need updating. Also, if you adopt this, we should loop in @mDuo13 to make sure he's aware.
There was a problem hiding this comment.
Coincidentally, the first if can be simplified further with only a bit of extra work:
Add #include <ripple/protocol/STAccount.h> at the top, then change the if to:
if ((*sleAccount)[~sfRegularKey] == idSigner)
This works because the left-hand side is a boost::optional<STAccount> and it has an overloaded operator== that ensures that the comparison succeeds iff the optional is seated.
There was a problem hiding this comment.
Are we sure we want to change the return codes? It involves both (1) changing the implementation in Transactor and (2) copying any test that asserts tefBAD_AUTH_MASTER or tefMASTER_DISABLED to run once with the amendment disabled and once with it enabled.
There was a problem hiding this comment.
The other downside is less granular errors, leaving users to wonder: which of the N possible ways was this a bad authorization?
There was a problem hiding this comment.
Alright, I restored tefMASTER_DISABLED and changed tests expecting tefBAD_AUTH_MASTER to expect tefBAD_AUTH.
| @@ -250,7 +250,7 @@ class Env_test : public beast::unit_test::suite | |||
| { | |||
| using namespace jtx; | |||
|
|
|||
| Env env(*this); | |||
| Env env{*this, supported_amendments() | fixDisabledRegularKey}; | |||
There was a problem hiding this comment.
Because fixDisabledRegularKey is in supported_amendments(), is it necessary to explicitly add it in here?
There was a problem hiding this comment.
I want to be robust against changes in supported_amendments. To be honest, I'm surprised that the tests aren't already like this, because it meant I had to add - fixDisabledRegularKey to some existing tests. I prefer that new code not have to change existing tests.
There was a problem hiding this comment.
Amendments aren't removed from supported_amendments very often, so I hadn't really considered the need. I agree about new code not changing existing tests in general, but when you're changing functionality, sometimes you gotta do what you gotta do. 😄
thejohnfreeman
force-pushed
the
1721-regular-master-keys
branch
from
5706330 to
12136c4
Compare
|
I will write something that everyone already knows, by the way. This bug will kill the master key first, or even if you set a regular key from Bob or Charlie to Alice's address, the first thing I've identified is the reverse procedure of Alice setting to Alice's public address also success, this is of course not an error code in the process of disable_master flag after this. So I think the nature of the problem is this: return a ERR_duplicate_key, or at the time of error recovery code such as RPE_duplicated_key is appropriate. Recently I am good report here to the owner of the wallet. |
|
The problem is as trivial as a mosquito bite, and it won't even be an exploit. Still, if you care about this boring and tedious tweak, we should know this word. The really beautiful code is simple and powerful. the same is true for the name. |
| { | ||
|
|
||
| if ((*sleAccount)[~sfRegularKey] == idSigner | ||
| || (!isMasterDisabled && idAccount == idSigner)) |
There was a problem hiding this comment.
Keeping in mind that this is the new flow, I believe that we are making the logic a little complicated to follow. I would prefer to see this block take the following shape:
if (ctx.view.rules().enabled(fixDisabledRegularKey))
{
// Signature made using the regular key: accepted
if ((*sleAccount)[~sfRegularKey] == idSigner)
return tesSUCCESS;
// Signature made using the master key: accepted if not disabled
if (!sleAccount->isFlag(lsfDisableMaster) && idAccount == idSigner)
return tesSUCCESS;
// Signature made with a key that's not authorized to sign
return tefBAD_AUTH;
}
I understand that this "removes" a theoretically useful error message but I highly doubt that the distinct error message is useful in practice. Whether the failure was because the master key is disabled or because the wrong key was used doesn't seem particularly actionable. I can imagine a scenario (a client that knows both the master and the regular key tries to sign with the master key first and, if disabled, transparently resigns with the regular key) but honestly, I find that flow broken.
The cost of having the extra result is, to be sure, minor but it also complicates (if only slightly) one of the most critical parts of the code: checking whether a given public key is authorized to sign for a particular account. In that context, every single instruction matters. Every single additional if is an opportunity to get it wrong or mess something up.
There was a problem hiding this comment.
I'll say that I do find @nbougalis's reformulation significantly easier to read and reason about, especially with the comments above each condition. I wonder is we can keep both this readability and the tefMASTER_DISABLED? Consider something like...
if (ctx.view.rules().enabled(fixDisabledRegularKey))
{
// Signature made using the regular key: accepted
if ((*sleAccount)[~sfRegularKey] == idSigner)
return tesSUCCESS;
// Signature made using the master key: accepted if not disabled
if (!isMasterDisabled && idAccount == idSigner)
return tesSUCCESS;
// Signature made using disabled master key
if (isMasterDisabled && idAccount == idSigner)
return tefMASTER_DISABLED;
// Signature made with a key that's not authorized to sign
return tefBAD_AUTH;
}
There was a problem hiding this comment.
I like the readability that comes with splitting up the success checks (especially because they can be commented separately), but I don't necessarily think we need to remove the other error message. For example,
if (ctx.view.rules().enabled(fixDisabledRegularKey))
{
// Signature made using the regular key: accepted
if ((*sleAccount)[~sfRegularKey] == idSigner)
return tesSUCCESS;
// Signature made using the master key: accepted if not disabled
if (!isMasterDisabled && idAccount == idSigner)
return tesSUCCESS;
// Signature made using the master key: reject if disabled
if (isMasterDisabled && idAccount == idSigner)
return tefMASTER_DISABLED;
// Signature made with a key that's not authorized to sign
return tefBAD_AUTH;
}
But I don't feel very strongly about changing it either. I also realize that with separate checks, we can verify with code coverage that each case was tested individually.
There was a problem hiding this comment.
I went ahead and made the changes @scottschurr and @ximinez suggested because they're easy. At this point, I don't think the reasons for removing tefMASTER_DISABLED are compelling enough to go through and rip it out.
There was a problem hiding this comment.
Thanks, @scottschurr, @ximinez, @thejohnfreeman.
|
👍 - will need squashing together, once we're ready to merge this. |
|
It is surprising that this problem has been neglected for two months. |
|
It'll still need to be activated, but that's up to the validators, not the people that write the code here. In code, the issue is solved already. |
(You'll want to ignore white space in the diff to read the new tests and test changes.)
What should we name the amendment? I defaulted to
fix1721based on a pattern among some of the other names, naming it after our internal JIRA issue, but I would prefer a name that (1) tries to describe the change and (2) is not tied to an issue number (given that our issue platform may change).Fixes JIRA-1721.