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

Sanity check secrets in freeradius::client #163

Merged
merged 2 commits into from
May 25, 2021

Conversation

nward
Copy link
Collaborator

@nward nward commented May 24, 2021

Clients can accept secrets with a newline as part of the string, which causes the RADIUS system to restart and.. fail to start, because the resulting config is something like:

secret = "foo
bar"

This PR implements a basic check which looks for a negative answer to "is this entire string not \n chars", and then raises an error. I've implemented the logic like this so we can add other "illegal" chars easily if we find them.

@nward nward added this to the Version 4 milestone May 24, 2021
I've implemented the logic like this so we can add other "illegal" chars easily if we find them.
djjudas21
djjudas21 previously approved these changes May 24, 2021
@djjudas21
Copy link
Owner

I haven't done this myself so I'm not quite sure... but is it possible to create a new data type with this validation that can be used in place of String? It could be useful if referenced in other places or maybe even in passwords.

@nward
Copy link
Collaborator Author

nward commented May 24, 2021

Hmm, I'm not actually sure how to create custom variable types in Puppet. I've definitely created custom resource types, but that's different.
Maybe an easy fix would be a custom function, which validates the parameter it's passed?
It occurs to me we should be using Sensitive for passwords, but I'm not sure what interop between versions is like.

I wonder if passwords would have different valid chars in different places?

I had a quick google, I'm surprised there isn't a String subtype which validates a regex.

I think this sort of thing is a good thing to look in to as a mini refactor project - would that work? I can create an issue for it, and we can track it for the future.

@djjudas21
Copy link
Owner

I may well have imagined this feature, so don't worry too much about it!

@amateo
Copy link
Collaborator

amateo commented May 24, 2021

No, you haven't imagine it, it's real. You can create a data type defining it in a directory named types. For example,

amateo@amateo-EXCALIBUR:$ cat profile/types/present_absent.pp 
type Profile::Present_absent = Enum['present', 'absent']

this a definition I have for a type named Profile::Present_absent and you can use it anywhere, for example in parameter declarations, just like any othe standard type. You have a lot of other examples in the stdlib module, for example.

What I don't know if it is possible is to define it like a string without \n characters, maybe something like this:

type Freeradius::Secret = Pattern[/[^\n]+/]

@nward
Copy link
Collaborator Author

nward commented May 24, 2021

Ohh nice! Go team :)

Ok, let me test this and refactor it with something like that in the next day or so.

… with Pattern, and also create a type for passwords
@nward
Copy link
Collaborator Author

nward commented May 24, 2021

OK - I've implemented 2 types: Freeradius::Password and Freeradius::Secret. I've done these separately, as I figure we might want to get a bit more specific about what each is allowed in the future - I'm not sure if a RADIUS secret has char limits, or length limits, for example.

I've set the types for a handful of classes and resource types to match, and have created test cases for each of these.

@amateo as it turns out, we didn't even need to look outside this repo for examples of Pattern - you had done several 4 years ago!

Copy link
Owner

@djjudas21 djjudas21 left a comment

Choose a reason for hiding this comment

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

Excellent, looks great

@nward nward merged commit 8225c28 into djjudas21:master May 25, 2021
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