Suggested recommendations for test, dev env, supply chain and error handling #62

Sans-Atout · 2024-08-25T21:05:08Z

Hi, everyone,

A few months ago, I had to produce a guide on how to develop securely in Rust.
This guide was a great help and one of my main sources.
So I thought I'd propose a few rules and a few changes that I think are r 8000 elevant after my research.

I apologise in advance for any mistakes I may have made.
I've tried using grammar checkers but I'm not sure I've corrected them all.

Here is a summary of the changes I am proposing :

Use of a private registry [9838274]
Supply chain security [a67bbce]
Improve unsafe block's checking ((False positives seem to have disappeared since the issue Forbid unsafe code #10 was written) [47c3727]
Improving best practice in error handling [e8be56f]
Good practice for writing tests [02e20a8]
Added the fact that you can now force lints in Cargo.toml [8c7b07f]

I hope these changes will be useful and I'd be more than happy to discuss them.

`+` Best practice in the use of an internal registry `+` Best practice for using several registry in a project `+` Publishing a project in an internal registry `+` Setting up a default registry for a developer `+` Adding french and english version

`+` Bests practices added for supply chain security `+` Adding french and english version

`+` Add a tool for checking the use of `unsafe` blocks in a program's supply chain `+` Adding french and english version `^` Translation error corrected in one of the links

`+` Test to comply with RFC1236 `+` Error API recommendation `+` French and english version

`^` Updating SUMMARY.md `^` Updating 01_introduction.md `+` Best practice in testing added

`.` Correct grammar error in french `+` Second method added for blocking `unsafe` blocks

polazarus

Thanks for the work. I think there is still work to be done here to polish the wording and some thinking about finding the right compromise.

In particular, private registry are not just a security device...

src/en/01_introduction.md

polazarus · 2025-02-13T15:32:47Z

src/en/02_devenv.md

+
+It is possible to configure `cargo` to retrieve data from multiple registries. This is done by adding the registers to the `.cargo/config.toml` file.
+
+```toml


IDK if it is really interesting. It seems a bit out of scope to speaks about this kind of configuration here.

polazarus · 2025-02-13T15:35:12Z

src/en/02_devenv.md

+
+> ### Rule {{#check DENV-REGISTRY-DEFAULT | Setting up a default registry}}
+>
+> In the event that an internal registry is deployed, it is advisable to set the organisation's internal registry as the default Rust registry.


Is that really reasonable?
It is only working for this kind of curated mirror registry.

polazarus · 2025-02-13T15:37:11Z

src/en/02_devenv.md

+Second, it is possible to use a solution that has already been built. A relatively exhaustive list, validated by the rust project, can be found on the [cargo project wiki pages](https://github.com/rust-lang/cargo/wiki/Third-party-registries
+)
+
+> ### Rule {{#check DENV-REGISTRY-LC | Internal registry management processes}}


I am not sure if it's really reasonable in the general case.

Enterprise may use private registry only to protect IP.

While I see the interest of this kind of vetted mirror, I am not sure it's the way to go in general.
Vetting through cargo-deny whitelist or other means seems more practical.

src/en/04_language.md

polazarus · 2025-02-13T15:48:24Z

src/en/03_libraries.md

+[Cargo-geiger] is a tool maintained by the Rust security working group.
+Its aim is to detect the use of the `unsafe` block in a project's supply chain.
+
+The results have three levels: 


I tried to change the formatting of this section via commit [53e5e0e] but I'm not sure that was what was requested.
If not, could you explain in more detail what formatting is expected?

src/en/08_testfuzz.md

polazarus · 2025-02-13T15:53:41Z

src/en/08_testfuzz.md

+cargo test
+```
+
+> ### Rule {{#check TEST-IMPL | Check that the public behaviour of the API is as expected}}


Lots of common libraries do not possess separate integration tests.

While providing out of crate test are a small plus, I am not sure of the interest of duplicating every test already played in the unit tests there.

I see what you mean. Rereading the rust book, I learn that when you run the cargo test command, it ensures that ‘examples’ are compiled correctly.

So I think that, if you agree, I can modify this recommendation to ask that the integration tests be done earlier via the examples. That way, the little extra security also becomes a great help to the developer.

polazarus · 2025-02-13T15:59:42Z

src/en/08_testfuzz.md

+> 
+> This also ensures that the API is user-friendly.
+
+### Implementing a trait


Will have to check on the current best practice

They more static_asserts than tests.
In particular, a simple const is now enough to make them.

const _: () = { const fn test_eq_trait<X: Eq>() {} test_eq_trait::<i32>(); // while <f32> fails };

I don't have a clear preference on how to implement them.
I think that these checks are still tests because I can't find their final use in code otherwise.

src/en/04_language.md

`^` Moving few word from british english to american english to improve overall consistency of the guide `^` Moving TODO in 01_introduction.md due to an oversight in initial PR `^` Changing from TEST-INTERNE to TEST-UNIT to comply with rust book `-` Removing second-person pronouns in 02_devenv.md

`^` Replacing an example of code that could be considered duplicated `^` Correction of the formatting of the three cargo-geiger output types/levels

`^` Adding explanation for check TEST-CFG

`-` Check LANG-ERRDO may be considered obsolete (cargo issues a warning if the try macro is used)

Sans-Atout · 2025-02-15T21:49:00Z

Hello,
First of all, many thanks for taking the time to review my PR and for your pertinent comments.

Regarding the comments on registries, I prefer to give a global answer here rather than in each comment. I completely understand your point of view, but I don't agree with it. In my view, having a private registry can help protect a company's IP but also make an attack on the supply chain more complex.

Although cargo-deny is a very good tool (and I think we need to add some recommendations on how to use it), it doesn't fully fulfil the role of a private registry. Indeed, since crates.io allows the deletion of crates, the use of a private registry can protect a company while the situation is resolved.

However, I agree that this is not a general thing but more recommendations to be adopted in the event that a high level of security is required.
Maybe it would be relevant for me to remove these recommendations from my PR while I open an issue in this repo? This would allow for more opinion/debate on the issue.

hg-anssi · 2025-06-13T07:27:53Z

src/en/02_devenv.md

@@ -250,3 +250,77 @@ There exist other useful tools or `cargo` subcommands for enforcing program
 security whether by searching for specific code patterns or by providing
 convenient commands for testing or fuzzing. They are discussed in the following
 chapters, according to their goals.
+
+## Using an internal registry


A less involving recommendation could be simply to track Cargo.lock in the SCM. I think this suffices to prevent dependencies overloading using corrupted registry.

Then, using its own registry protect developers from upgrading dependencies (via cargo update for instance) to a nasty one. But maybe this could be a second (more involving) recommendation.

hg-anssi · 2025-06-13T07:37:22Z

src/en/02_devenv.md

+There are two ways of deploying an internal registry.
+One way is to develop your own solution. This must meet the API requirements [described here](https://doc.rust-lang.org/cargo/reference/registry-web-api.html).
+
+Second, it is possible to use a solution that has already been built. A relatively exhaustive list, validated by the rust project, can be found on the [cargo project wiki pages](https://github.com/rust-lang/cargo/wiki/Third-party-registries
+)


Another solution may be to simply use git reference in Cargo.toml, in a golang fashion.

This recommendation seems largely easier to follow since deploying its own git server is a common practice in a industrial context.

hg-anssi · 2025-06-13T08:06:06Z

src/en/03_libraries.md

+
+> Rule {{#check LIBS-VET | Priority use of libraries that have been audited}}
+>
+> It is advisable to use the `cargo-vet` tool to prioritise the use of libraries which have been audited by third parties.


Suggested change

> It is advisable to use the `cargo-vet` tool to prioritise the use of libraries which have been audited by third parties.

> It is advisable to use the `cargo-vet` tool to prioritize the use of libraries which have been audited by third parties.

hg-anssi · 2025-06-13T08:06:41Z

src/en/03_libraries.md

+
+> ### Rule {{#check LIBS-SUPPLY-CHAIN | Check developers implicitly trusted}}
+>
+> The `cargo-supply-chain` tool must be used to find out who your organisation implicitly trust to run your project.


Suggested change

> The `cargo-supply-chain` tool must be used to find out who your organisation implicitly trust to run your project.

> The `cargo-supply-chain` tool may be used to find out contributors of the project dependencies.

hg-anssi · 2025-06-13T08:25:39Z

src/en/03_libraries.md

+### Cargo vet / crev
+
+[Cargo-vet] is a tool developed by the Mozilla Foundation that allows you to check whether the libraries you can use have been audited by trusted third parties.
+
+> Rule {{#check LIBS-VET | Priority use of libraries that have been audited}}
+>
+> It is advisable to use the `cargo-vet` tool to prioritise the use of libraries which have been audited by third parties.
+
+Security audits can be created using a tool called [Cargo-crev]. The use of this tool will not be detailed in this guide.
+
+For more information, please consult the tool's [official documentation].


This interesting tool, if added to the guide, needs more explanations to understand how it works.

Moreover, I think it involve sophisticated processes (audits, trust relation, level of trust, ...) which should be carefully audited before proposing it in the guide.

hg-anssi · 2025-06-13T08:50:59Z

src/en/03_libraries.md

+## Unsafe code in libraries
+
+[Cargo-geiger] is a tool maintained by the Rust security working group.
+Its aim is to detect the use of the `unsafe` block in a project's supply chain. The results have three levels:
+
+- `🔒` means that no `unsafe` usage found and the create declares #![forbid(unsafe_code)]
+- `❓` means that no `unsafe` usage found and the create missing #![forbid(unsafe_code)]
+- `☢️` means that `unsafe` usage found
+
+> ### Rule {{#check LIBS-UNSAFE | Check *unsafe* code in dependencies}}
+>
+> Use the `cargo-geiger` tool to check that uses of the `unsafe` block comply with the recommendations described in the following section of this guide.
+
+
+[cargo-geiger]: https://github.com/geiger-rs/cargo-geiger


Whether a dependency uses unsafe code does not necessarily indicate that this dependency has vulnerabilities. The recommendation may be more general and warns developers against unaudited libraries.

hg-anssi · 2025-06-13T09:06:10Z

src/en/04_language.md

+[lints.rust]
+unsafe_code="forbid"


hg-anssi · 2025-06-13T09:16:35Z

src/en/04_language.md

+> ### Recommendation {{#check LANG-ERR-FLAT | root positioning of type `Error` }}
 >
-> The `?` operator should be used to improve readability of code.
-> The `try!` macro should not be used.
+> It is advisable to publicly position this type at the root of your API. For example: `crate::Error`.
+
+To do this, you can flatten the `Result` and `Error` types using the following piece of code placed at the root of the `src/lib.rs` or `src/main.rs` file: 
+
+```rust
+pub use error::Error;
+```
+
+The advantage of this technique is that, in your code, you can make the position of the type in your project agnostic for the user or for yourself.


I think this is more an API design practice, and not a security practice. Thus I'm not sure whether this has to be recommended in this guide.

hg-anssi · 2025-06-13T09:28:12Z

src/en/08_testfuzz.md

+> If a test is marked as ‘ignore’, it will no longer be possible to run it even if you specify its name using the `cargo test <test_name>` command. 
+> 
+> The only way it can be run is to remove the `#[ignore]` above it.


No, ignored tests can be run with command cargo test -- --ignored

hg-anssi · 2025-06-13T09:29:22Z

src/en/08_testfuzz.md

+It is also possible to ignore certain tests if they take too long to run. This can be done by adding `#[ignore]` above the test function.
+
+```rust
+#[cfg(test)]
+mod tests{
+	#[test]
+	fn test_non_ignore(){ ... }
+
+	#[ignore]
+	#[test]
+	fn test_ignore() { ... }
+}
+```


I'm not sure that talking about ignored test is a security concern.

hg-anssi · 2025-06-13T09:34:55Z

src/en/08_testfuzz.md

+> ### Recommendation {{#check TEST-UNIT | Test all functions}}
+>
+> It is advisable to test all the functions of your program, even those that may seem the most trivial.
+> 
+> This way, if a future modification causes a side-effect that modifies the behavior of another function, you will notice it much more quickly.
+> This also helps to limit the number of bugs as early as possible. 
+
+```rust
+// private function
+fn my_function(){
+	... // Your code here
+}
+
+#[cfg(test)]
+mod tests{
+	#[test]
+	fn test_my_function(){
+		... // Your tests here
+	}
+}
+```
+


This is not feasible in practice. Moreover, I think this recommendation does not target the good level of advice: the guide should not be a manual for computer science student and should assume a minimum level of general programming knowledge.

hg-anssi · 2025-06-13T09:51:58Z

src/en/08_testfuzz.md

+
+> ### Recommendation {{#check TEST-TRAIT | Create tests to ensure that certain traits are implemented for structures/enums}}
+>
+> In certain contexts, it is necessary for certain struct or enum to implement particular traits. 


Explain why.

For instance, to check that exposed API is complete.

hg-anssi

Thank you @Sans-Atout for your important contribution. I left you comments.

@hg-anssi

`^` Change of the `LIBS-AUDIT-UNSAFE` recommendation to take into account the recommendations of @hg-anssi `^` Change of the `TEST-UNIT` recommendation to take into account the recommendations of @hg-anssi `-` Deletion of the section explaining what ignored tests are `-` Deletion of recommendation LANG-ERR-FLAT `^` Modification for English and French versions

`-` Deletion of recommendations on the use of an internal registery `-` Deletion of `cargo-vet` tool recommendations

Sans-Atout added 6 commits August 19, 2024 21:02

Supply chain best practices

a67bbce

`+` Bests practices added for supply chain security `+` Adding french and english version

Improve unsafe blocks checking

47c3727

`+` Add a tool for checking the use of `unsafe` blocks in a program's supply chain `+` Adding french and english version `^` Translation error corrected in one of the links

Improving error handling's best practices

e8be56f

`+` Test to comply with RFC1236 `+` Error API recommendation `+` French and english version

Testing's best practice

02e20a8

`^` Updating SUMMARY.md `^` Updating 01_introduction.md `+` Best practice in testing added

Minor improvement before PR

8c7b07f

`.` Correct grammar error in french `+` Second method added for blocking `unsafe` blocks

polazarus reviewed Feb 13, 2025

View reviewed changes

Sans-Atout added 4 commits February 14, 2025 22:53

Slight improvement in style and formatting

53e5e0e

`^` Replacing an example of code that could be considered duplicated `^` Correction of the formatting of the three cargo-geiger output types/levels

Extra explanation added

ca549e3

`^` Adding explanation for check TEST-CFG

Deleting an obsolete recommendation

ac24449

`-` Check LANG-ERRDO may be considered obsolete (cargo issues a warning if the try macro is used)

Sans-Atout requested a review from polazarus March 1, 2025 09:31

hg-anssi reviewed Jun 13, 2025

View reviewed changes

< 9E81 div aria-label="Only reviews by reviewers with write access count toward mergeability" aria-role="image" data-view-component="true" class="TimelineItem-badge">

hg-anssi reviewed Jun 13, 2025

View reviewed changes

src/en/04_language.md

Comment on lines +74 to +75

[lints.rust]

unsafe_code="forbid"

Copy link

hg-anssi Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

hg-anssi reviewed Jun 13, 2025

View reviewed changes

hg-anssi suggested changes Jun 13, 2025

View reviewed changes

Sans-Atout added 2 commits June 23, 2025 08:09

Deletion of recommendations requiring further discussion

0bdd824

`-` Deletion of recommendations on the use of an internal registery `-` Deletion of `cargo-vet` tool recommendations

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Suggested recommendations for test, dev env, supply chain and error handling #62

Suggested recommendations for test, dev env, supply chain and error handling #62

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!


		It is possible to configure `cargo` to retrieve data from multiple registries. This is done by adding the registers to the `.cargo/config.toml` file.

		```toml

	> It is advisable to use the `cargo-vet` tool to prioritise the use of libraries which have been audited by third parties.
	> It is advisable to use the `cargo-vet` tool to prioritize the use of libraries which have been audited by third parties.

	> The `cargo-supply-chain` tool must be used to find out who your organisation implicitly trust to run your project.
	> The `cargo-supply-chain` tool may be used to find out contributors of the project dependencies.

Suggested recommendations for test, dev env, supply chain and error handling #62

Are you sure you want to change the base?

Suggested recommendations for test, dev env, supply chain and error handling #62

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!