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.

Sign up for GitHub

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

Fixes #148 #149

Merged
merged 6 commits into from
Mar 21, 2023
Merged

Fixes #148 #149

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5d071d2
Fixes #https://github.com/denisenkom/pytds/issues/148
aersam Mar 17, 2023
1bfca1e
respect ' instead of "
aersam Mar 17, 2023
37543b2
Merge branch 'master' of https://github.com/denisenkom/pytds into tls…
aersam Mar 21, 2023
76b8a4d
add tests
aersam Mar 21, 2023
acf0822
bug fix
aersam Mar 21, 2023
c9b929f
fix tests
aersam Mar 21, 2023
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
18 changes: 13 additions & 5 deletions src/pytds/tls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,19 @@ def validate_host(cert, name):
ext = cert.get_extension(i)
if ext.get_short_name() == b'subjectAltName':
s = str(ext)
Copy link
Owner

Choose a reason for hiding this comment

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

Please refactor this code: extract logic inside this if branch into its own method is_san_matching(san: str, host_name: str): bool.

Also add unit tests for that method for following conditions:

  • exact SAN match
  • wildcard SAN match
  • SAN mismatch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a number of tests in tls_san_test.py, I hope thats correct this way?

# SANs are usually have form like: DNS:hostname
if s.startswith('DNS:') and s[4:] == s_name:
return True

# TODO handle wildcards
items = s.split(',')
for item in items:
dnsentry = item.strip()
# SANs are usually have form like: DNS:hostname
if dnsentry.startswith('DNS:') and s[4:] == s_name:
return True
if dnsentry.startswith('DNS:*.'): # support for wildcards, but only at the first position
Copy link
Owner

Choose a reason for hiding this comment

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

It would be better to extract SAN host name suffix before previous condition, and then check host name for exact match and for wildcard match. Instead of checking for DNS: prefix twice

Copy link
Contributor Author

@aersam aersam Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I did that refactoring, hope it's fine now

afterstar_parts = dnsentry[6:]
afterstar_parts_sname = '.'.join(s_name.split('.')[1:]) # remove first part of dns name
if afterstar_parts == afterstar_parts_sname:
return True

# TODO check if wildcard is needed in CN as well
return False


Expand Down