Allow nominator for CI checks on banned calls

[lukehinds]

Calls to nopanic / unwrap can be allowed by using #[allow_ci]

[lkatalin]

I think this would need to check for #[allow_ci] in the previous line, unless I am misreading this.

[lukehinds]

Hmm, this seems to work well:

https://github.com/keylime/rust-keylime/runs/1413393155?check_suite_focus=true

The previous line is just to go over the banned dict , I think this could likely be optimized a bit better using regexp, but we don't need to worry about performance too much

[lukehinds]

it looks like it was not even working before (it was allowing a load of unwraps).

[lkatalin]

Ah, yeah, that's a lot of unwraps - it's good to have this as an improvement.

I am seeing the line numbers reported as one off from where the unwrap() is called, though, ex. File crypto.rs on line number 200 calls banned function: unwrap() while I see the unwrap() on 201: https://github.com/keylime/rust-keylime/blob/master/src/crypto.rs#L201. Not a big deal but easy to change.

[lkatalin]

My concern is more about the #[allow_ci] being placed on the previous line from panic! or unwrap() - would that be caught here?

[lukehinds]

I see now, so I planned to have it on the same line as the instance of panic! / unwrap. This is how we do it on the bandit project

https://github.com/PyCQA/bandit/blob/9b4cf91e51b2cea6754bbb80f85bf968c92d4201/doc/source/config.rst#suppressing-individual-lines

key.unwrap() #[allow_ci]

Here is an example of it working:

(Note, this is before you're change to increment the line by +1 so it records the correct line number).

[lkatalin]

Ah, this makes much more sense now. Thanks for the clarification!

[lukehinds]

no worries. what you would you like to do around the naming convention? I went with allow_ci rather than a mention of panics (as it also covers unwraps as well)? I am open to suggestions here.

[lkatalin]

I think allow_ci is fine for now; if we end up allowing more types of banned calls, we may want to differentiate later. Alternatively allow_banned is possibly a little more descriptive.

[lkatalin]

@lukehinds I created a branch to test this by using #[allow_ci] on one of the unwraps() here, but the line was still flagged by the nopanic test as File cmd_exec.rs on line number 136 calls banned function: unwrap(). Is this a different way than how you were thinking of using it? I was thinking of something like this.

There are still improvements to be made, ex. it's a bit awkward to put this on the line immediately before an unwrap() instead of immediately before a multi-line statement or function using unwrap() like a real Rust attribute, but it works for now without a lot of parsing.

[lkatalin]

Also, the more I think about it, it may be more readable to call this something like #[allow_panic].

[lkatalin]

Thanks @lukehinds

Edit: Do we need CI to pass for this to merge? And / or should I make a separate patch to either get rid of the unwraps() or mark them as acceptable?

[lukehinds]

Thanks @lukehinds

Edit: Do we need CI to pass for this to merge? And / or should I make a separate patch to either get rid of the unwraps() or mark them as acceptable?

We can merge this and then follow up with a patch. I am fine to mark all those unwraps as allow, I imagine a lot of those will be refactored out when we work more on the crypto side.

I will go ahead and merge this.

[lukehinds]

actually, looks like I need to fix these first (the repo is stopping admin overriding for mergeing)