Skip to content
This repository has been archived by the owner on Aug 6, 2023. It is now read-only.

Use salt when generating ids from ips #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jdpage
Copy link

@jdpage jdpage commented Feb 28, 2021

Implemented salting for IP addresses, per #6

@jdpage
Copy link
Author

jdpage commented Feb 28, 2021

I made the default behavior to create a new salt file, but that means that old installations which upgrade need to set the setting to "disabled" to retain the old behavior, which might not be what you want.

@makew0rld
Copy link
Owner

Thanks for adding this! Two things: As I told someone in #7, please make the salt config option a global variable rather than parsing the config file again. Secondly, don't ready from the salt file every time the function is called, that's inefficient. Just do all the salting logic in the Init and have a global salt variable.

@jdpage
Copy link
Author

jdpage commented Mar 2, 2021

Okay, yeah, I don't have a good excuse for why I implemented it that way. I'm gonna chalk it up to having an off day 😓

It now sets up a slice during init, leaving it empty if salting is disabled.

Oh, and I changed the permissions on the salt file to 0644, to match the permissions used elsewhere.

}
}

return ioutil.ReadFile(ipSaltPath)
Copy link
Owner

Choose a reason for hiding this comment

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

If we just created the salt, then this line will try to read from that file before it's even closed. It would be better to just return the salt after creating it on line 210, if there's no error.

@makew0rld
Copy link
Owner

Sorry for leaving this for so long. This project is not a top priority for me, as it's more of toy. But anyway I'm happy to merge this, just have one comment, see above.

@makew0rld
Copy link
Owner

@jdpage any interest in finishing this up?

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

Successfully merging this pull request may close these issues.

None yet

2 participants