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

Remove hard-coded default for RHS #10

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@nalind
Copy link
Contributor

commented May 3, 2016

Don't fall back to using a default LHS or RHS when the configuration file can't be read. Original report from https://bugzilla.redhat.com/show_bug.cgi?id=1332493

@kaduk

This comment has been minimized.

Copy link

commented May 3, 2016

LGTM

@achernya

This comment has been minimized.

Copy link
Owner

commented May 3, 2016

Hm, this seems to break the tests

Can't initialize hesiod library.
FAIL: hestest.conf

@nalind Can you take a look at the failure?

@nalind

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2016

Would it make more sense to continue setting a default LHS, erroring out if we can't read an RHS and also don't find one in the environment? Setting HES_DOMAIN during the tests should be enough for them.

@nalind nalind force-pushed the nalind:no_def_lhsrhs branch from 5d032ca to f51cf6c May 3, 2016

@nalind

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2016

Okay, giving that a try.

@nalind nalind changed the title Remove hard-coded defaults for LHS and RHS Remove hard-coded default for RHS May 3, 2016

@nalind nalind force-pushed the nalind:no_def_lhsrhs branch 3 times, most recently from 362b76d to 2e8be6a May 3, 2016

@nalind

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2016

Okay, switching back to setting a default LHS if we fail to read the configuration file, and erroring out later if there isn't an acceptable RHS value set in the environment, and making sure that HES_DOMAIN is set to "athena.mit.edu" when we run hestest, seems to work.

/* Make sure that the rhs is set. */
if (!ctx->rhs)
{
errno = ENOEXEC;

This comment has been minimized.

Copy link
@achernya

achernya May 3, 2016

Owner

Why ENOEXEC here?

This comment has been minimized.

Copy link
@nalind

nalind May 3, 2016

Author Contributor

It's copied from read_config_file(), though I have no idea why read_config_file() uses that particular error code. Would ENOENT make more sense?

This comment has been minimized.

Copy link
@achernya

achernya May 3, 2016

Owner

EINVAL or ENOENT make more sense

This comment has been minimized.

Copy link
@nalind

nalind May 3, 2016

Author Contributor

Okay, I'm leaning toward EINVAL because at that point we may well have managed to read a configuration file, so ENOENT could be confusing.

@nalind nalind force-pushed the nalind:no_def_lhsrhs branch from 2e8be6a to 3b0a6f9 May 3, 2016

Remove hard-coded defaults for LHS and RHS
Don't fall back to using a default LHS or RHS when the configuration
file can't be read.  Instead, return an error.
Original report from https://bugzilla.redhat.com/show_bug.cgi?id=1332493
@nalind

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2016

Hmm, the more I think about this, the more I'm leaning toward going back to just returning an error when we fail to open the file, and setting HESIOD_CONFIG to point to the in-tree hesiod.conf.sample file when we run hestest. That seems to be enough for it here. Opinions?

@achernya

This comment has been minimized.

Copy link
Owner

commented May 4, 2016

I would be okay with pointing hestest at a config file. That seems most correct if you're improving the unconfigured state.

@nalind

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2016

Okay, I'll switch this over to doing that.

@nalind nalind force-pushed the nalind:no_def_lhsrhs branch from 3b0a6f9 to 247e2ce May 4, 2016

@kaduk

This comment has been minimized.

Copy link

commented May 4, 2016

Yes, having the tests point to an explicit config file and not relying on default behavior seems like a fine plan.

@nalind

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2016

Okay, the changes are ready.

@carnil

This comment has been minimized.

Copy link

commented Jan 21, 2017

This has been assigned CVE-2016-10152

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.