Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First stab at a re-write. #1

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
190 changes: 190 additions & 0 deletions csrgen.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
#!/usr/bin/env python3
import argparse
import fnmatch
import re
import os
import subprocess
import readline
from jinja2 import Template, Environment, FileSystemLoader

file_loader = FileSystemLoader('.')
env = Environment(loader=file_loader)


description = """

Choose a reason for hiding this comment

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

PEP8 says constants should be named in all caps.

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

Choose a reason for hiding this comment

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

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

Simple script to handle generating Certificate Signing Requests (CSR)
This has the ability to generate CSRs with and without a provided key.
Also can handle multiple domains as Subject Alternative Name (SAN) records.
"""
config_template = """[ req ]
default_bits = 2048
distinguished_name = req_distinguished_name
prompt = no
req_extensions = v3_req
[ req_distinguished_name ]
{% if c is not none -%}
C = {{s}}
{%- endif %}
{%- if l is not none -%}
L = {{l}}
{%- endif %}
{%- if s is not none -%}
ST = {{s}}
{%- endif %}
{%- if o is not none -%}
O = {{o}}
{%- endif %}
{{' CN = {{ cn }}
[ v3_req ]
subjectAltName = @alt_names

[alt_names]
DNS.1 = {{ cn }}
{%- for domain in sans %}
{%- if domain %}
DNS.{{ loop.index +1 }} = {{ domain }}
{%- endif %}
{%- endfor %}
'}}
"""


def check_file(file):

Choose a reason for hiding this comment

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

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():

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

try:
os.stat(file).st_size > 1
except:
return False
else:
return file
Comment on lines +53 to +58
Copy link

@danielhoherd danielhoherd May 19, 2022

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

If it returns true how does that affect argparse value?



def check_domain(input):

Choose a reason for hiding this comment

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

Same comments as earlier:

  • what are you checking for? consider is_domain
  • what type is input?

Copy link
Owner Author

Choose a reason for hiding this comment

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

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?

if check_file(input):
return input, True
else:
domain_template = re.compile(
"^(?=.{1,255}$)(?!-)[A-Za-z0-9\-]{1,63}(\.[A-Za-z0-9\-]{1,63})*\.?(?<!-)$")
if domain_template.match(input):
return input.lower()
else:
raise argparse.ArgumentError(
"Unable to determine if this is a domain or a file. Please check your input.")


def parse_arguments():
parser = argparse.ArgumentParser(description=description,
formatter_class=argparse.RawTextHelpFormatter)
subparser = parser.add_subparsers(dest="command")

csr = subparser.add_parser("csr", help="Create CSR",
formatter_class=argparse.ArgumentDefaultsHelpFormatter)
csr.add_argument('-k', "--key", type=check_file, help='Specify a key')
csr.add_argument('-d', "--domain", type=check_domain, required=True,
help="Specify a domain or a file with list of domains")
csr.add_argument("-o", "--org", help="Organization Name")

config = subparser.add_parser("config", help="Create config file")
config.add_argument("-c", "--country", help="Country short code")
config.add_argument("-l", "--locality", help="Locality/City")
config.add_argument("-s", "--state", help="State Name")
config.add_argument("-o", "--org", help="Organization Name", required=True)

if parser.parse_args().command:
return parser.parse_args()
else:
parser.print_help()
exit()


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

Choose a reason for hiding this comment

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

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

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

print("Generating CSR configuration..")
if type(args.domain) is tuple:

Choose a reason for hiding this comment

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

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

domain_file, is_file = args.domain
with open(domain_file) as fp:
domain = fp.readline()

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

SANS.append(domain.strip().lower())
domain = fp.readline()
count += 1
else:
print("Too many domains! Please limit to 100")
exit(1)
print(CN)
else:
domain = args.domain
CN = domain.strip().lower()

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

Choose a reason for hiding this comment

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

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

generated_csr_config.write(tm.render(cn=CN, sans=SANS))
generated_csr_config.close()

return str(args.domain + '_csr_config')


def main(args):

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

if len(config_file_search) == 0:
print("No config files found please run with 'config -h' to create one")
exit()
elif len(config_file_search) > 1: # Check if there is more then one config file
if args.org:
# Check if any of the config files match the specified Org
if check_file({args.org + "_gen_config"}):
csr_config_out = gen_csr_config(
args, {args.org + "_gen_config"})
else:
print("Please specify Organization name with -o <Org>")
exit()
else: # There is only one found so assume its the right one\
csr_config_out = gen_csr_config(args, config_file_search[0])

# Validate key argument and file
if args.key:
if check_file(args.key):
try:
subprocess.call(['openssl', 'rsa', '-in',

Choose a reason for hiding this comment

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

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.

args.key, '-check'])
except:

Choose a reason for hiding this comment

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

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

Suggested change
except:
except subprocess.CalledProcessError:

print("Key specified is not a valid rsa key.")
else:
print("Key file specified doesn't exist.")
else:
print("No key specified. Creating a new one.")
subprocess.call(['openssl', 'genrsa', '-out',
args.domain + '.key', '2048'])
print("Key Generated")

if args.key:
print("Generating CSR")
subprocess.call(['openssl', 'req', '-new', '-config', csr_config_out,
'-key', + args.key, '-out', args.domain + '.csr'])
else:
print("Generating CSR")
subprocess.call(['openssl', 'req', '-new', '-config', csr_config_out,
'-key', args.domain + '.key', '-out', args.domain + '.csr'])

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

Choose a reason for hiding this comment

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

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

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

filename=config_file_name))
config_file = open(config_file_name, "w")

Choose a reason for hiding this comment

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

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

config_file.write(tm.render(c=args.country, l=args.locality,
s=args.state, o=args.org))
config_file.close()


if __name__ == '__main__':
main(parse_arguments())