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

Jump to bottom

SSL cleanups (with a few BSD odds & ends) #276

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

SSL cleanups (with a few BSD odds & ends) #276

wants to merge 7 commits into from

Conversation

dougnazar
Copy link
Contributor

Various patches cleaning up the SSL handling:

  • Free SSL structure on failed handshake (memory leak).
  • Ensure we can compile without any errors or deprecations.
  • Generate DH params natively on 3.x if needed (auto recommended)
  • Fix several warnings about using non-length versions of strcat, strcpy, sprintf, etc.
  • Move duplicated SSL code from nrpe & check_nrpe to separate ssl.c file

This has been compile tested on the following configurations:

  • Gentoo Linux (system)
    • openssl-3.0.8
  • Gentoo Linux (local installation)
    • openssl-1.0.0t
    • openssl-1.0.1u
    • openssl-1.0.2u
    • openssl-1.1.0l
    • openssl-1.1.1s
    • openssl-3.0.8
    • openssl-3.1.0
  • FreeBSD 13.2
    • libressl-3.6.2
    • openssl-1.1.1t
    • openssl30-3.0.8
    • openssl31-3.1.0
  • OpenBSD 7.3
    • LibreSSL 3.7.2
    • openssl-1.1.1t
    • openssl-3.0.8
    • openssl-3.1.0

It's been running in production (along with my other patches) on one of my Gentoo boxes. The others have been compile (and minimally run) tested only.

@Starow Starow mentioned this pull request Nov 15, 2023
Closed
@tsadpbb
Copy link

tsadpbb commented Jul 31, 2024 •
edited
Loading

Bike shedding a little bit, but could you add the ssl.o and the utils.o to the gitignore? Also, I noticed ssl.o doesn't get removed from make clean.

Edit: Could you also add generate_dh_params to the gitignore?

@tsadpbb
Copy link

tsadpbb commented Aug 1, 2024

With the Makefile command

check_nrpe: 
        $(srcdir)/check_nrpe.c utils.o $(SRC_INCLUDE)/utils.h $(CFG_INCLUDE)/common.h $(CFG_INCLUDE)/config.h $(SSL_OBJS)
	$(CC) $(CFLAGS) -o $@ $(srcdir)/check_nrpe.c utils.o $(SSL_OBJS) $(LDFLAGS) $(SOCKETLIBS) $(SNPRINTF_O) $(OTHERLIBS)

Do you think you would ever need to specify the directory that utils.o is in? Something like this

check_nrpe: $(srcdir)/check_nrpe.c $(srcdir)/utils.o $(SRC_INCLUDE)/utils.h $(CFG_INCLUDE)/common.h $(CFG_INCLUDE)/config.h $(srcdir)/$(SSL_OBJS)
	$(CC) $(CFLAGS) -o $@ $(srcdir)/check_nrpe.c $(srcdir)/utils.o $(srcdir)/$(SSL_OBJS) $(LDFLAGS) $(SOCKETLIBS) $(SNPRINTF_O) $(OTHERLIBS)

This applies to the nrpe command as well

@tsadpbb
Copy link

tsadpbb commented Aug 1, 2024

Should the auto_dh be enabled by default or should it be disabled by default to conform with backwards compatibility?

@dougnazar
Copy link
Contributor Author

Bike shedding a little bit, but could you add the ssl.o and the utils.o to the gitignore? Also, I noticed ssl.o doesn't get removed from make clean.

Edit: Could you also add generate_dh_params to the gitignore?

I did most of my testing in separate build directories so I didn't notice. I had just assumed all *.o files were ignored. I'll add a wild card and generate_dh_params.

I missed the ssl.o on clean. Easy enough to fix.

Do you think you would ever need to specify the directory that utils.o is in? Something like this

No. utils.o is in the build (current) directory, we only need to refer back to the source directory for *.c, *.h or any other shipped file.

Should the auto_dh be enabled by default or should it be disabled by default to conform with backwards compatibility?

It's a bit of an effort to remember, but I don't think I changed backwards compatibility here. It defaulted to always generating a DH key, I just switched to auto keying on setups that supported it. It should support all the same ciphers suites and connections to older versions as before.

I believe (sorry, it's been awhile) I tested various settings of ssl_use_adh as well as a few older versions of nrpe to check the legacy cases, but my testing definitely wasn't exhaustive.

@dougnazar
Don't compile utils.c twice, once for each program.
dc8eb69
@dougnazar
Free SSL structure on failed connection attempts.
9221a7a
@dougnazar
Fix various OpenSSL version compatibilities.
b391bcd 
Since 1.1.0 the library will auto initialize and on 3.x these functions are deprecated.
Use ERR_get_error instead of ERR_get_error_line_data since we don't use the extra options.
Detect if library supports SSL_OP_NO_TLSv1_1 before using.
@dougnazar
Rewrite OpenSSL detection, use auto DH setup, and properly support 3.x.
cb7512e 
Fix OpenSSL detection (don't include prefix and libraries may end in .a)
Add rpath to linker flags if libraries aren't in default location.
Switch to using recommended auto setup of DH parameters on OpenSSL 1.1.0+.
Rewrite OpenSSL 3.0+ generation of DH parameters to use new API.
Use OpenSSL headers to detect version since may mismatch detected binary.
Move generation of DH parameters to Makefile.
@dougnazar
Several small misc fixes.
c4701f6
@dougnazar
Fix various warnings.
bf9db0b 
Remove uses of strcat, strcpy, sprintf, etc.
Fix warnings about unused variables.
@dougnazar
Move common SSL functions to separate file.
16f4ba5
tsadpbb
tsadpbb reviewed Aug 5, 2024
View reviewed changes
macros/ax_nagios_get_ssl
Comment on lines +99 to +103
AC_ARG_ENABLE([auto_dh],
AS_HELP_STRING([--disable-auto-dh],[disables using builtin DH parameters (if available) and generates custom parameters]),
auto_dh=no,
auto_dh=yes)

Copy link

Choose a reason for hiding this comment

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

I believe, because of this segment of code here, that unless you provide --disable-auto-dh then auto_dh will equal yes. So I think to have the default behavior be "disabled by default" then it should look something like

Suggested change
AC_ARG_ENABLE([auto_dh],
AS_HELP_STRING([--disable-auto-dh],[disables using builtin DH parameters (if available) and generates custom parameters]),
auto_dh=no,
auto_dh=yes)
AC_ARG_ENABLE([auto_dh],
AS_HELP_STRING([--enable-auto-dh],[enables using builtin DH parameters (if available) and generates custom parameters]),
auto_dh=yes,
auto_dh=no)

Copy link

Choose a reason for hiding this comment

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

Naturally, correct me if I am wrong about this

Copy link

Choose a reason for hiding this comment

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

Although it might be better if auto_dh is enabled by default anyways. I'll try testing how different versions play with each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, it defaults to using auto DH keying on setups that support it, but only if need_dh is true. So with the same configure options the software should function the exact same. The source of the DH parameters (auto or self-generated) has no affect. OpenSSL recommends using auto keying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsadpbb tsadpbb tsadpbb left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dougnazar @tsadpbb