Assemble a PR checklist for code reviewers

[ionut-arm]

The amount of information that we need to be aware of when reviewing a PR is increasing, and given the possibility of breaking the sync between code and documentation in "everyday PRs", it would be good to have a checklist to help with this.

The checklist will be published in the book and will cover:

• Rough areas in the book where we should check after some specific types of changes
• Guidelines to follow for the code, either to have idiomatic Rust usage or things that we restrict for ourselves
• General guidelines for our development lifecycle (e.g. commits should be signed, testing should be in place for new functionality)

[ionut-arm]

I won't add any points that are fairly obvious - e.g. if we add a new provider, that will certainly involve adding a bunch of stuff in the book and it's a fairly rare occurence.

[ionut-arm]

I'll be using this comment as the base for the list and will keep updating it.

Code-wise

Follow Rust API guidelines

Usage of unsafe blocks, .unwrap(), .expect(...), panic!(...) etc. should be strictly limited to extreme cases and documented properly.

Abstract types should be preferred to generic representations - e.g. instead of representing PSA algorithms as a bitfield like in the spec, a rich Rust-native type was used.

Buffers should be zeroed out if they contain any sensitive data.

Logs should not contain sensitive data, and should only contain detailed data and error information if configured so.

New functionality is properly tested.

Threat model

The threat model should be reviewed if:

• Avenues for communication between the service and the rest of the system are created or modified
• Components that deal with authenticating requests are created or modified
• Key namespacing is altered in any way

Special care should also be taken around the bits of code that enforce key policies.

Documentation

If changes are made to the authentication process, the API overview, system architecture and authenticators pages should be checked.

If new response codes are added, please review the status codes page.

If improving support for one of the providers, please check the service API coverage page.

If large changes are made (including additions or deletions) to the source code structure, please check the associated page.

If changes are made to the placement (in the filesystem) of service components or utility files, please check the build and run, secure installation pages.

If changes are made to the CLI arguments (Cargo features or other arguments parsed by the service binary) that need to be passed to run Parsec or its tests, please check the build and run, secure installation and testing pages.

If new kinds of tests are added, please check the testing page and its child.

[hug-dev]

Thanks!! It looks good, I have a few questions:

Abstract types should be preferred to generic representations.

What do you mean?

Buffers should be zeroed out.

Maybe only the ones potentially containing confidential information?

Usage of unwrap, expect, panic etc. should be strictly limited to extreme cases and documented properly.

I would also add something similar for usage of unsafe.

[ionut-arm]

Abstract types should be preferred to generic representations.

What do you mean?

Well, that we should use a new type to represent ApplicationIdentity instead of just using a String, for example. That sort of thing. Will try to make it more explicit.

Buffers should be zeroed out.

Maybe only the ones potentially containing confidential information?

👌

Usage of unwrap, expect, panic etc. should be strictly limited to extreme cases and documented properly.

I would also add something similar for usage of unsafe.

👌

[hug-dev]

Well, that we should use a new type to represent ApplicationIdentity instead of just using a String, for example. That sort of thing. Will try to make it more explicit.

ahh yes agree then!

[ionut-arm]

Done here: parallaxsecond/parsec-book#68