This is a great start!

Hmm, so I know in the project description we called it "AWS Credential Checker" but I wonder if we shouldn't make this more generic. It would be nice to be able to add on other common credential types (SSH keys, etc.)

In your description, you say it "adds an integration". The lingo we use in review bot is "adds a tool"

This docstring almost reads as if "AWS Credential Checker" is a third-party tool that can be run. Let's say something like "Review Bot tool to check for hardcoded credentials"

For imports, we have a standard based on Python's style guide. Imports should be broken up into 4 possible sections separated by newlines.

imports from __future__

standard library imports (that would be your re import)

imports from third-party libraries not in the standard library. This can include imports from other packages that we maintain, such as rbtools.

imports from "this package" (meaning anything in reviewbot)


Within that, we do any full-package imports first (import ...), followed by any from imports. All of that should be alphabetized. What you have is simple enough that most of those rules don't apply, but the sections need to be reorganized as such:

from __future__ import unicode_literals

import re

from reviewbot.tools import Tool

Same here as the module docstring.

There's nothing in this that requires using the raw string (r prefix). This should also use single-quotes instead of double.

We're compiling the regex once per file, but of course it will never change. Can we define an __init__ method for the tool, and store the compiled regex as an attribute?

We can avoid writing the file out to disk and reading it back in. Let's do something like:

data = f.patched_file_contents

if not data:
  return

for i, line in enumerate(data.splitlines()):
    ...

I don't think we need to list the actual contents of the keys in the comment message, since it will be associated with the line and show up with the context. That lets us simplify the message to something warning that they're potentially hard-coding secret credentials in their code.
In the future, one thing to be really careful about is the use of str in Python code. On Python 2, str (aka bytes) is a bytestring and unicode is a unicode string. On Python 3, str is unicode and bytes is bytes, which makes a lot more sense. Any time there's a potential mix of string types we need to be really careful, especially since Python 3 treats such mixing as an error but Python 2 will happily attempt to cast silently.

If we make the above change to use patched_file_contents then this point is moot, but one cool thing about Python is you can use files as a context manager:

with open(path) as f:
    ... do stuff with f...


When the code exits the context of that with block, it will automatically close the file. This happens even if there's an exception that occurs within the block, and helps prevent open file descriptor leaks.

It seems like we could probably just do this inside the loop that is doing all the matches.

Filenames should always be lowercase and consistent with files in the same directory. We don't have any others with capitalization or underscores.
Let's call this one rbcredentialcheck.py.

It'd be nice to include what services this checks for.

This is missing a docstring.

We always want to use super to call parent methods.

I have some notes for improving this.
1. Raw strings
Let's prefix each string with r, which is for a raw string. This allows suff like \n to literally match the text \n, not a newline. It's often handy for regexes, and helps avoid mistakes in the future.

2. Alphabetizing
Let's alphabetize the keys, so it's more clear where any new ones should go.

3. Performance
Currently, every time the tool is initialized, we reconstruct the pattern from the strings in a dictionary. I think there are two things we should do:

Check for and store the pattern on the class during initialization (so we only construct it once per process) — this is probably best moved to __init__ in the process.
Build this as one big regex.

The latter would look like:

pattern = re.compile(
    r'''(

    # AWS Access Key
    ((A3T[A-Z0-9]|AKIA|AGPA|AIDA|AROA|AIPA|ANPA|ANVA|ASIA)[A-Z0-9]{16})
    |

    # AWS Secret Key
    ([aA][wW][sS].*[0-9a-zA-Z/+]{40})
    |

    ..etc..
    )''', re.X)


This is faster and allows expressions to be more free-form and have room to breathe.

4. Alphabetization
I'm tempted to say we shouldn't trust that the content is going to always be in the same casing. We don't want to miss something just because someone pasted a SSH key that had bEGIN RSA PRIVATE KEY.
Ideally we'd use Python 3.7+'s ability to mark subsections of regexes as case-insensitive, but we can't. We could just require it for the whole string by using the re.I option on re.compile().
Interested in hearing your thoughts on that.

Is there anything more specific and easy to match than this? It feels pretty generic.

There shouldn't be ablank line after function/method-level docstrings. Just file/class.

Here's an interesting thing. We cannot assume UTF-8 here. Plenty of files are in other encodings.
So what we should do is leave this as a byte string, and have the compiled regex also be a byte string. Then we don't have to think about encodings. This is also faster.

Every time we call self.pattern.search(line), we're re-fetching pattern from self. For a 5,000 line file, that means 5,000 attribute accesses.
Instead, let's pull pattern out of self before the loop, and access it directly as a local variable.

There should always be a blank line between statements (like match = ...) and blocks (if ...).

When formatting multi-line strings, we prefer using parens and having the space at the end of a line instead of the beginning.
However, this can just be provided inline to f.comment().

There should be a blank line between the statement and block.

We should log if this fails. To do this, at the top, import logging and do:

logger = logging.getLogger(__name__)


This gives us a logger that will namespace any messages, making it clear which tool this is coming from. You can then call logger.exception(...) with suitable information.

The name doesn't reflect the conventions used elsewhere. We may also want to distinguish it from other tools by prefixing it. How about rbcredentialcheck?

Looking pretty great!

Instead of "if a credential has been committed", perhaps "check for hard-coded security credentials"?
Same in the docstrings throughout this.

Hmm, I'm not sure about this comment text. Perhaps something like "This appears to be a hard-coded credential, which is a potential security risk" ?

I don't think any of the other tools wrap f.comment in an exception handler. Were you encountering some problem?

This text can wrap to a wider width (80 columns). Also same comment about the description text as in the credential checker source.

Exclamation points are rarely if ever appropriate for use in UI text, even for security notices. Let's just use a period.

Even though it's > 80 columns, let's put this all on one line. Wrapping like this is hard to read and error-prone.

Can we add a blank line between these two?

Just a thought regarding the syntax error: maybe try using rb""" ... """ with triple double-quotes?

We're only using the match once, so this can be combined:

if pattern.search(line):
    f.comment(...)

Looking very solid. I just see one nit left to pick.

This is only used in one place, so we might as well skip assigning the local variable and just use self.pattern.search() below.

Looks good to me!

I noticed a similar situation in the ShellCheck Tool and figured that it would be worth highlighting here as well. 
In short, this will most likely have compatibility issues with python 2.7 and thus should probably be written as super(CredentialCheckerTool, self).__init__().

Ship It!

Ship It!