auditing attribute macro for FRAME pallets

[sam0x17]

Based on the forum discussion at https://forum.polkadot.network/t/rfc-trustless-in-code-auditing-annotations/988 and some internal discussions, it has become clear there is significant desire within FRAME to have auditing machinery that will allow us to annotate pallets with some attribute macro containing a hash code generated from the AST nodes that make up the actual contents of the pallet. This would look something like the following:

#[pallet]
#[audited(E0C5E21EB7AD91AA)]
pub mod pallet {
  ...
}

At compile-time we would generate a new hash for the AST nodes that make up the pallet, and if the new hash doesn't match the one recorded in the audited attribute, we would issue a compile error saying that the pallet needs to be re-audited. Then presumably the #[audited()] line would be commented out and the auditing team would add a new hash code and uncomment this line when they finish their audit.

We would also add a new version of construct_runtime!, perhaps construct_audited_runtime! that will only allow audited pallets in the runtime and will issue a compile-time error in the event that an unaudited pallet is included in the runtime.

Together, these two features will give pallet developers a better way to maintain and track the auditing status of their pallets, and runtime developers will have increased peace of mind knowing that all the pallets they are using in their runtime are audited if they opt into using the construct_audited_runtime! syntax.

We could also do a little interaction with the recently merged "pallet dev mode" feature (#12536) such that a compiler error will also be issued if a pallet with dev mode enabled is included in a construct_audited_runtime! runtime, if desirable.

[bkchr]

We would also add a new version of construct_runtime!, perhaps construct_audited_runtime! that will only allow audited pallets in the runtime and will issue a compile-time error in the event that an unaudited pallet is included in the runtime.

If you do this, then you would require that pr that is only merged when it is audited. Imagine we are using construct_audited_runtime in Polkadot. We would not be able to merge anything without it being audited. General this should be fine and would just require to adapt the process. However, another thing would be testing. This stuff should not get into the way of playing/adding new features or whatever. So, I think this shouldn't be its own macro call, it should more be some kind of attribute that you can set in construct_runtime based on some feature being enabled. In Polkadot we for example have the on-chain-release-build feature for this.

A little bit more off topic, the issue should be renamed. Auditing is never "trustless", at least not in my pov.

[sam0x17]

I agree a feature flag might be useful for toggling this off. As for the trustless thing, I’m more referring to the annotations as a way of verifying that the code you have right here matches what was at some point audited versus trusting your mastery of git blame and what some old PR says about the auditing status of the pallet, but point taken. My point is more that anything short of this you have to trust that the code you have right here is the same code some external thread is talking about, assuming we have a rigid process around changing audited() lines

On Wed, Nov 9, 2022 at 3:46 AM Bastian Köcher @.***> wrote:

We would also add a new version of construct_runtime!, perhaps construct_audited_runtime! that will only allow audited pallets in the runtime and will issue a compile-time error in the event that an unaudited pallet is included in the runtime.

If you do this, then you would require that pr that is only merged when it is audited. Imagine we are using construct_audited_runtime in Polkadot. We would not be able to merge anything without it being audited. General this should be fine and would just require to adapt the process. However, another thing would be testing. This stuff should not get into the way of playing/adding new features or whatever. So, I think this shouldn't be its own macro call, it should more be some kind of attribute that you can set in construct_runtime based on some feature being enabled. In Polkadot we for example have the on-chain-release-build feature for this.

A little bit more off topic, the issue should be renamed. Auditing is never "trustless", at least not in my pov.

— Reply to this email directly, view it on GitHub https://github.com/paritytech/polkadot-sdk/issues/242, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOE4LI7NEU2WU7T5PXPKDLWHNQGJANCNFSM6AAAAAAR3CJG7E . You are receiving this because you were assigned.Message ID: @.***>

[burdges]

You cannot use negative or default features here, as they complicate people causally playing, and cargo features are additive, but an automated process could use cargo build --features requireaudits of course.

[ggwpez]

Why do we need rust features for this?
Can we not add some syntax to the construct_runtime! macro that asserts all pallets to be audited in a test?

[sam0x17]

I agree and am not really clear why we would need a feature for this. If someone is playing/tinkering, they simply wouldn't use construct_audited_runtime! (or simply wouldn't pass "audited" or whatever argument to construct_runtime! if we decide to do it that way).

If they wish to tweak an existing pallet, they merely have to remove or comment out the #[audited()] macro line above that pallet definition, and then they can tweak away.

I also plan to include the expected and actual hash codes in the compiler error in the event that the hash codes don't match, so they could also easily change the hash code manually based on this output in their local tinkering code instead of doing the above, if they so wish.

But yeah, I don't really see why we need to bring features into this, just wanted to appease if everyone thought that was necessary for some reason.

[bkchr]

The moment you start changing a pallet, polkadot would stop compiling. This is a really shitty experience. It is not about "tinkering" of other users, it is about our work flow. Like you add some new pallet or fix something in a pallet and you want to test this. If there would be an extra construct runtime, it would require that you always change the macro name back or you would need to update the hash after each time you pressed a key on your keyboard. This is a really shitty dev experience.

I already had similar experiences with people requiring no warnings on a pallet level. This was an insanely annoying experience. When I want to change code, I don't want change unrelated code to achieve whatever requirement that isn't coming from rust itself.

Tldr, don't destroy the dev experience.

[sam0x17]

I definitely understand where you're coming from @bkchr -- the last thing I would want to do with this is create a shitty developer experience. That said, I think the workflow becomes pretty clear -- if you want to "open up" an already-audited pallet for editing, you simply comment out the #[audited()] line for that pallet, or, as you said, we add some feature flag for this, which I'm happy to do if you think that would make things less painful.

Another interesting idea would be to only allow the audit hash code compile-time errors to occur if/when one of those pallets is included in a construct_audited_runtime! call. So that way even if you tamper with a pallet, you won't get a compile error until you go to compile a runtime that is using construct_audited_runtime!. Perhaps this would be a better experience and address your concerns? I think it would be a reasonable compromise ;)

[burdges]

That's too heavy a change for new developers. It's harmful security theater for experienced developers.

you simply comment out the #[audited()] line for that pallet

This workflow breaks the whole process obviously. lol

you won't get a compile error until you go to compile a runtime that is using construct_audited_runtime!

Integration tests exist, so this cannot work. Also profiling exists, so #[not(cfg(feature = "debug"))] cannot work either.

We should simply write a log file of audit violations, and leave the log file empty if no violations, so then deployment tooling can fail if the log file exists.

A non-default "enable audit checks" features almost works. We'd warn to only enable via --features never Cargo.toml, but surely fools would keep enabling in Cargo.toml anyways, and breaking the whole workflow for themselves and others.

Aside form being atrocious dev experience, a "disable audit checks" feature obviously cannot work because any create could silently disable it for the whole build.

[sam0x17]

@burdges thanks for spelling this out, I have a much clearer picture now!

In that case, I think it would be quite reasonable to have deployment tooling fail if a log file indicating auditing failures exists. Do you know where approximately in the deployment code a good injection point might be for this? I'm not familiar with the deployment machinery.

[burdges]

I donno, but let's see what others more involved with the code and process say. I just wanted to make it really clear that pushing the actual check to late in the packaging/deployment pipeline makes the most sense from a security point of view too. :)