search

bgentry / go-netrc

netrc file parser for Go programming language.
http://code.google.com/p/go-netrc/
Other
26 stars 15 forks source link

Parsing might be too strict #1

Open bgentry opened 10 years ago

bgentry commented 10 years ago

@deafbybeheading had the following messed up .netrc file:

machine me.test.com
  login me@test.com
  password REDACTED
machine me2.test.com
  login me@test.com
  password 
REDACTED api.heroku.com
  login me@test.com
  password REDACTED
machine code.test.com
  login me@test.com
  password REDACTED

Where each use of REDACTED was a password. Somehow geemus/netrc was able to read hid creds from this file, but go-netrc wasn't.

I'm actually not sure that we should handle this. If the .netrc is clearly invalid, you should have to fix it. The netrc manpage certainly makes me think it's invalid.

bgentry commented 10 years ago

Maybe @geemus knows something about this.

geemus commented 10 years ago

Not sure off hand I'm afraid. kr did the implementation work there (I just did a couple tweaks and was told to maintain). It certainly does appear invalid and is surprising that it wouldn't blow up more spectacularly even in the ruby netrc thing.

bgentry commented 10 years ago

For some reason @mfine had a similarly messed up .netrc file. I'm thinking this project might need to be just ignore stuff it can't parse (or at least offer that option).

geemus commented 10 years ago

It is a mixed bag. Ignoring un-parseable stuff is great as long as that isn't the pair you are trying to look up (in which case having it tell you it doesn't exist is confusing in it's own right). Not sure if fail fast is better or fail accurate, so to speak. May depend on how widespread these issues come to be.

billmoritz commented 6 years ago

I've run in to this issue with Heroku and Terraform. Using the Heroku cli to login to an SSO endpoint creates method and org keywords for the machine api.heroku.com section. 

machine api.heroku.com
  login email@domain.com
  password xxxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxx
  method sso
  org orgname

Any time I use a go application that uses this library I get Error parsing netrc file at "/Users/wmoritz/.netrc": line 5: keyword expected; got method

apparentlymart commented 6 years ago

Hello all! I'm a member of the Terraform team, and we just saw a new issue hashicorp/terraform#18610 that seems to be covering the same problem as reported by @billmoritz above.

My thought to address this would be for the parser to attempt graceful recovery when it encounters something it can't parse. Specifically, when encountering an invalid token, it could discard the entry it's currently parsing and then seek forward to the next line that starts with the machine token, and resume parsing from there. That would avoid it misinterpreting machine sections containing directives it can't understand while still allowing the remainder of the file to be processed as expected.

I agree with the above discussion that it's not ideal for an error to just be silently ignored, and I have a possible compromise for that too: although the existing FindMachine method does not return an error (it assumes all error handling happened during ParseFile) -- perhaps a new method alongside it could be defined with the signature func (string) (*Machine, error) and then have this return an error if there was a parse error for that host. That way a calling application can get a good result in the case where the unparseable entry is not what was being requested, but generate an error if it was.

Does that seem like a reasonable path forward?