Don't rewrite non-gitea public keys

[lunny]

This will fix #424

[bkcsoft]

can we defer f.Close() instead?

[lunny]

No. We have to close the file and then rename it from tmp to normal.

[bkcsoft]

you can still defer f.Close(), it will just not do anything if it's already closed :)

[lunny]

So it's no need to use defer f.Close().

[bkcsoft]

defer f.Close() is good practice, if I'm not familiar with the code-base I'd assume that it defers it and then we have a bug 🙁

[tboerger]

👍 for defer, it's common practice to make sure the file gets always closed.

[tboerger]

That way you can also skip all the f.Close() calls.

[lunny]

done.

[lunny]

It seems drone down?

[strk]

If I read the code correctly you're using full path to the gitea binary as the "tag", correct ?
This means if you change the gitea path and rewrite authorized keys you end up with duplicate keys, is that correct ? How does sshd behave in that case ? Is that something we need to handle somehow ? For example logging a warning if a possibly conflicting key is found ?

[travnick]

I'd like to suggest to add new config option like "KEY_PREFIX_ID".
Then use prefix for keys like "gitea-KEY_PREFIX_ID". Now you are free to move around gitea without issues with duplicated keys.

By default KEY_PREFIX_ID should be an UUID that is generated during installation.

[lunny]

I did not consider to move the place of the Gate binary.

[strk]

As a contribution for the discussion, gitolite marks its section with start and end block comments: ``` # gitolite start ... # gitolite end ```

[strk]

The prefix configuration would suffer from the same issue: if it changes, the match would fail. Let's say this version LGTM - can be improved later. At least this version does not require changing existing deploys, and should work as long as the SSH access to repos work.

[bkcsoft]

I would read the file line-by-line, appending each to the list of keys we wanna write. Sort the list, step through and remove duplicates.

Pseudo-code as fuck because I'm tired

keys := getListsAsStringSlice()
f, _ := os.Open("authorized_keys")
for line := f.ReadLine() {
  keys := append(keys, line)
}
strings.Sort(keys)
outKeys := []string{keys[0]}
for i := 1; i < len(keys) - 1; i++ {
  if keys[i] != keys[i-1] {
    outKeys = append(outKeys, keys[i])
  }
}
f.Seek(0, io.SeekStart)
f.Write(outKeys)

[lunny]

@bkcsoft You have to consider backup if the machine is down when it's writing keys. So rename is not an ignoreable step. Or maybe we need copy not rename.

[lunny]

@travnick do you mean on the comment?

[travnick]

@lunny what are you asking about exactly?

[bkcsoft]

@lunny copy sounds better :)

[lunny]

@tboerger @bkcsoft @strk please confirm.

[bkcsoft]

This will skip every other line no?

[tboerger]

This will skip always the next line if the current line is the prefix

[tboerger]

Isn't it easier to have a start and an end marker line gitolite?

[lunny]

It's also easy to this one. but it seems no need to add end.

[tboerger]

But this implementation is harder to understand, at least that's my opinion.

[lunny]

But it works. We can improve it in v1.2.

[strk]

Is this assuming all authorized_keys belong to gitea ?

[lunny]

If it's an old gitea, of course because every time you add a new public key. it will remove other non-gitea public key.

[lunny]

For a new gitea, this will not be executed.

[bkcsoft]

We can improve it in v1.2.

No, we'd have to change for v2.0 since that would be a breaking change

[lunny]

Where is the breakage?

[bkcsoft]

The breakage is in the expected behaviour. Anyhow, this is LGTM to me 🙂