Skip to content
New issue

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

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

Already on GitHub? # to your account

Add support for fingerprint alg #137

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bazsi
Copy link
Member

@bazsi bazsi commented Jun 2, 2024

This branch allows you to specify the fingerprint-alg() to be used for validating key fingerprints in trusted-keys()

Fixes: syslog-ng/syslog-ng#4976

bazsi added 2 commits June 2, 2024 19:19
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
Copy link
Member

@alltilla alltilla left a comment

Choose a reason for hiding this comment

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

The last commit lacks the signoff part. I don't think it is a good idea if I write it for you and force push, so please add it yourself. Thanks!

The code looks good to me. We need a newsfile entry, but I can make it for you.

bazsi added 2 commits June 3, 2024 16:45
…ngerprint-alg

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi bazsi force-pushed the add-support-for-fingerprint-alg branch from c5fc4f9 to 4cc0353 Compare June 3, 2024 14:55
@bazsi
Copy link
Member Author

bazsi commented Jun 3, 2024

I've rebased, fixed the signoff and added the news entry. btw, @MrAnno this PR now allows to trust specific keys, even if their X.509 validation fails.

The code did not support it thus far, but I think this has always been the intention. Currently, trusted-keys() will only trust keys that are otherwise trusted, which does not make much sense to me.

The documentation is not that clear on this topic either (@fekete-robert).

@MrAnno
Copy link
Member

MrAnno commented Oct 12, 2024

From #136:

I was not completely right here: if the fingerprint alg is chosen well, it is suitable to replace the CA-based verification, and that can be a perfectly valid use case.

With this new implementation, we should not default to SHA1, and we should consider not letting users set a weak hash function.
To avoid any confusion, I'd remove the old option and introduce a new one, for example:

trusted-fingerprints(sha256, "aa:bb:cc", ...)

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

Successfully merging this pull request may close these issues.

trusted-keys: support a secure hash algorithm
3 participants