First stab at a re-write. #1

drahamim · 2022-05-19T16:17:51Z

No description provided.

lrosenman · 2022-05-19T16:33:24Z

Looks good, but I'm not a python language lawyer.

csrgen.py

danielhoherd · 2022-05-19T18:11:57Z

csrgen.py

+    return str(args.domain + '_csr_config')
+
+
+def main(args):


Seems to me like there's too much implementation logic in the main() function. I'd try to move a lot of this to functions and keep only business logic in main(). Maybe you could create one function for each args.command.

Here's an example of how it could be implemented https://github.com/pre-commit/pre-commit/blob/main/pre_commit/main.py#L349-L355

+1 to keeping main() down to a small set of function calls.

Also consider trapping the case where command is not in the expected set of commands. Don't really see where that is documented and/or output to the user.

Co-authored-by: Daniel Hoherd <[email protected]>

danielhoherd · 2022-05-19T18:21:10Z

csrgen.py

+    try:
+        os.stat(file).st_size > 1
+    except:
+        return False
+    else:
+        return file


Suggested change

try:

os.stat(file).st_size > 1

except:

return False

else:

return file

try:

return os.stat(file).st_size > 1 or file

except FileNotFoundError:

return False

Actually that may not be right. A docstring would be helpful to define the desired behavior. I think I misinterpreted it.

This is suppoed to check that the file exists and is not empty.

danielhoherd · 2022-05-19T18:24:50Z

csrgen.py

+                try:
+                    subprocess.call(['openssl', 'rsa', '-in',
+                                    args.key, '-check'])
+                except:


I think this should be, but I'm not 100% sure.

Suggested change

except:

except subprocess.CalledProcessError:

table-delete · 2022-05-19T18:59:32Z

csrgen.py

+"""
+
+
+def check_file(file):


The name check_file is ambiguous... what are you checking? Consider a name file_exists so your code reads more naturally, ex. if file_exists():

Aaaand I just realized this is already a library function: os.path.exists() for any path, pathlib.Path.is_file() for files in particular.

Also the file parameter is a bit ambiguous. Are you looking for a file_name with a string representation, a path-like object, an actual file object?

Can document in code by checking your parameter first thing in the code: if not isinstance(file, str): raise TypeError.

In general, good practice to check your parameters before using them, to make debugging easier.

table-delete · 2022-05-19T19:05:40Z

csrgen.py

+    except:
+        return False
+    else:
+        return file


I see two different types being returned by this function: bool False and str file.

Choose one and stick with it. Looks like you want bool here.

If it returns true how does that affect argparse value?

table-delete · 2022-05-19T19:06:28Z

csrgen.py

+        return file
+
+
+def check_domain(input):


Same comments as earlier:

what are you checking for? consider is_domain

what type is input?

Two things. Check if it is a domain or if it is a file with domains. Maybe I should create 2 flags rather then combining them in one?

table-delete · 2022-05-19T19:07:54Z

csrgen.py

+                    CN = domain.strip().lower()
+                    domain = fp.readline()
+                    count += 1
+                elif 1 <= count <= 100:


Suggest creating a named constant at the top of the file instead of burying 100 here, to make maintenance easier. ex. MAX_DOMAINS = 100

table-delete · 2022-05-19T19:08:51Z

csrgen.py

+env = Environment(loader=file_loader)
+
+
+description = """


PEP8 says constants should be named in all caps.

https://peps.python.org/pep-0008/#constants

NIT: mixed use of single-quote and double-quotes throughout this file.

table-delete · 2022-05-19T19:10:29Z

csrgen.py

+def gen_csr_config(args, config_file):
+    tm = env.get_template(config_file)
+    print(config_file)
+    SANS = list()


All-caps variable names are generally used for constants. Suggest sans here.

https://peps.python.org/pep-0008/#constants

table-delete · 2022-05-19T19:11:41Z

csrgen.py

+    print(config_file)
+    SANS = list()
+    print("Generating CSR configuration..")
+    if type(args.domain) is tuple:


instanceof() is permissive of subclasses whereas type() is requires exact match. Consider using instanceof() in most cases.

table-delete · 2022-05-19T19:30:33Z

csrgen.py

+    if type(args.domain) is tuple:
+        domain_file, is_file = args.domain
+        with open(domain_file) as fp:
+            domain = fp.readline()


Suggest using for domain in fp: instead of this combo of readline() and a while-loop. Code ends up much simpler & easier to maintain.

table-delete · 2022-05-19T19:33:32Z

csrgen.py

+        if args.key:
+            if check_file(args.key):
+                try:
+                    subprocess.call(['openssl', 'rsa', '-in',


Here and elsewhere: suggest checking & printing stderr and/or return code from any subprocess call / run. This allows you to debug why something fails much more easily.

table-delete · 2022-05-19T19:34:13Z

csrgen.py

+def main(args):
+    if args.command == 'csr':
+        # Look for csrgen config files
+        config_file_search = fnmatch.filter(os.listdir(), "*_gen_config")


I see _gen_config in quite a few places. Consider replacing it with a named constant for maintainability.

table-delete · 2022-05-19T19:35:05Z

csrgen.py

+    if args.command == 'config':
+        tm = Template(config_template)
+        config_file_name = args.org + "_gen_config"
+        print("Creating config template file: {filename}".format(


NIT use an f-string instead of "".format()

ex. print(f"creating {config_file_name}")

table-delete · 2022-05-19T19:35:49Z

csrgen.py

+        config_file_name = args.org + "_gen_config"
+        print("Creating config template file: {filename}".format(
+            filename=config_file_name))
+        config_file = open(config_file_name, "w")


Prefer with open() as f: to calling open and close separately.

table-delete · 2022-05-19T19:37:21Z

csrgen.py

+        domain = args.domain
+        CN = domain.strip().lower()
+
+    generated_csr_config = open(args.domain + '_csr_config', 'w')


Prefer with open() as f: to calling open and close separately.

drahamim added 8 commits May 16, 2022 16:54

inital logic of csrgen script

2008f84

formatting fixes

1d40207

fix typo

1bd42ea

adding config file template

94d1afe

finished main logic.

e859bf7

Working version

1193197

removing test files

5355a4d

minor code cleanup

4cc6880

danielhoherd reviewed May 19, 2022

View reviewed changes

csrgen.py Outdated Show resolved Hide resolved

danielhoherd reviewed May 19, 2022

View reviewed changes

csrgen.py Outdated Show resolved Hide resolved

danielhoherd reviewed May 19, 2022

View reviewed changes

Apply suggestions from code review Spelling fixes

cfa115f

Co-authored-by: Daniel Hoherd <[email protected]>

danielhoherd reviewed May 19, 2022

View reviewed changes

table-delete reviewed May 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First stab at a re-write. #1

First stab at a re-write. #1

drahamim commented May 19, 2022

lrosenman commented May 19, 2022

danielhoherd May 19, 2022

danielhoherd May 19, 2022

table-delete May 19, 2022

table-delete May 19, 2022

danielhoherd May 19, 2022 •

edited

Loading

danielhoherd May 19, 2022

drahamim May 19, 2022

danielhoherd May 19, 2022

table-delete May 19, 2022

table-delete May 19, 2022

table-delete May 19, 2022

table-delete May 19, 2022

drahamim May 19, 2022

table-delete May 19, 2022

drahamim May 19, 2022

table-delete May 19, 2022

table-delete May 19, 2022

table-delete May 19, 2022

table-delete May 19, 2022

table-delete May 19, 2022

table-delete May 19, 2022

table-delete May 19, 2022

table-delete May 19, 2022

table-delete May 19, 2022

table-delete May 19, 2022

table-delete May 19, 2022

First stab at a re-write. #1

Are you sure you want to change the base?

First stab at a re-write. #1

Conversation

drahamim commented May 19, 2022

lrosenman commented May 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhoherd May 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhoherd May 19, 2022 •

edited

Loading