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

Implement fallback to ASN1_STRING_data #362

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

k0ekk0ek
Copy link
Contributor

@k0ekk0ek k0ekk0ek commented Jul 29, 2024

The version used of OpenSSL used by the opencsw project is too old and does not offer ASN1_STRING_get0_data. Implement falling back to ANS1_STRING_data instead. Also fixes sk_GENERAL_NAME_pop_free not being called if the subject alternative name matched previously. Apart from that, I also removed duplication of the common name, I'm not sure it adds useful debugging info.

@k0ekk0ek k0ekk0ek force-pushed the test-for-asn1-string-get0-data branch from f40bf56 to 2c1e088 Compare July 29, 2024 12:29
@k0ekk0ek k0ekk0ek force-pushed the test-for-asn1-string-get0-data branch from 2c1e088 to fca1eec Compare July 29, 2024 13:45
Copy link
Member

@wcawijngaards wcawijngaards left a comment

Choose a reason for hiding this comment

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

The code looks good. Avoids useless allocations, and it still has debug output.

@k0ekk0ek
Copy link
Contributor Author

k0ekk0ek commented Aug 1, 2024

@bilias, this slightly updates your work on #337. If you can spare the time, can you verify everything still works for you? Thanks in advance.

@k0ekk0ek
Copy link
Contributor Author

k0ekk0ek commented Aug 1, 2024

The person reporting the error via email confirmed NSD compiles on Solaris again with both GCC and the Sun compiler, so we're good there.

@bilias
Copy link
Contributor

bilias commented Aug 1, 2024

@k0ekk0ek

Just tested https://github.com/k0ekk0ek/nsd/tree/test-for-asn1-string-get0-data
and it passed my tests both for allow and deny access depending on certificate (CN and SAN).

Code looks good to me and I like the simplification of the code after your changes.

@k0ekk0ek
Copy link
Contributor Author

k0ekk0ek commented Aug 1, 2024

Thanks for testing @bilias!

@k0ekk0ek k0ekk0ek merged commit b88421b into NLnetLabs:master Aug 1, 2024
4 checks passed
@k0ekk0ek k0ekk0ek deleted the test-for-asn1-string-get0-data branch August 1, 2024 21:33
@bilias
Copy link
Contributor

bilias commented Aug 1, 2024

@k0ekk0ek
My only concern is the removal of those DEBUG statements, especially after reading Andreas mail and his last two mentioned notes.

Trying to find out why TLS-auth does not work can be tricky some times, especially with the default output log of openssl which sucks (in my opinion). That's why I did all that fuzz and added all those extra DEBUG.

Maybe some errors deserve a (better) VERBOSITY log.

@k0ekk0ek
Copy link
Contributor Author

k0ekk0ek commented Aug 2, 2024

@bilias, I've created a separate issue for it (#369) and we can do it for the next release. The resource leak and build issue on Solaris have been fixed, so I'd like to focus on 4.10.1 first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants