Merge pull request #19 from keybase/david/better-code-comments

More comments on methods throughout the codebase

Author: ddworken

src/cmd/keybaseca/keybaseca.go

@@ -122,7 +122,7 @@ func generateAction(c *cli.Context) error {
 	}
 	captureControlCToDeleteClientConfig(conf)
 	defer deleteClientConfig(conf)
-	err = sshutils.Generate(conf, c.Bool("overwrite-existing-key") || os.Getenv("FORCE_WRITE") == "true", true)
+	err = sshutils.Generate(conf, c.Bool("overwrite-existing-key") || os.Getenv("FORCE_WRITE") == "true")
 	if err != nil {
 		return fmt.Errorf("Failed to generate a new key: %v", err)
 	}

src/keybaseca/config/config.go

@@ -116,36 +116,45 @@ func validatePath(path string) error {
 	return nil
 }
 
+// A Config struct that pulls transparently from the environment
 type EnvConfig struct{}
 
 var _ Config = (*EnvConfig)(nil)
 
+// Get the location of the CA key
 func (ef *EnvConfig) GetCAKeyLocation() string {
 	if os.Getenv("CA_KEY_LOCATION") != "" {
 		return shared.ExpandPathWithTilde(os.Getenv("CA_KEY_LOCATION"))
 	}
 	return shared.ExpandPathWithTilde("/mnt/keybase-ca-key")
 }
 
+// Get the keybase home directory. Used if you are running a separate instance of keybase for the chatbot. May be empty.
 func (ef *EnvConfig) GetKeybaseHomeDir() string {
 	return os.Getenv("KEYBASE_HOME_DIR")
 }
 
+// Get the keybase paper key for the bot account. Used if you are running a separate instance of keybase for the chatbot.
+// May be empty.
 func (ef *EnvConfig) GetKeybasePaperKey() string {
 	return os.Getenv("KEYBASE_PAPERKEY")
 }
 
+// Get the keybase username for the bot account. Used if you are running a separate instance of keybase for the chatbot.
+// May be empty.
 func (ef *EnvConfig) GetKeybaseUsername() string {
 	return os.Getenv("KEYBASE_USERNAME")
 }
 
+// Get the expiration period for signatures generated by the bot.
 func (ef *EnvConfig) GetKeyExpiration() string {
 	if os.Getenv("KEY_EXPIRATION") != "" {
 		return os.Getenv("KEY_EXPIRATION")
 	}
 	return "+1h"
 }
 
+// Get the list of keybase teams configured to be used with the bot.
 func (ef *EnvConfig) GetTeams() []string {
 	split := strings.Split(os.Getenv("TEAMS"), ",")
 	var teams []string
@@ -158,6 +167,7 @@ func (ef *EnvConfig) GetTeams() []string {
 	return teams
 }
 
+// Get the location for the bot's audit logs. May be empty.
 func (ef *EnvConfig) GetLogLocation() string {
 	return os.Getenv("LOG_LOCATION")
 }
@@ -166,14 +176,18 @@ func (ef *EnvConfig) getStrictLogging() string {
 	return strings.ToLower(os.Getenv("STRICT_LOGGING"))
 }
 
+// Get whether or not strict logging (see env.md for a description of this feature) is enabled
 func (ef *EnvConfig) GetStrictLogging() bool {
 	return ef.getStrictLogging() == "true"
 }
 
+// Get the Keybase chat location configured to be used for all communication. A chat channel consists of
+// team.subteam#channel-name. May be empty.
 func (ef *EnvConfig) getChatChannel() string {
 	return os.Getenv("CHAT_CHANNEL")
 }
 
+// Get the team used for all communication. May be empty.
 func (ef *EnvConfig) GetChatTeam() string {
 	if ef.getChatChannel() == "" {
 		return ""
@@ -185,6 +199,7 @@ func (ef *EnvConfig) GetChatTeam() string {
 	return team
 }
 
+// Get the channel used for all communication. May be empty.
 func (ef *EnvConfig) GetChannelName() string {
 	if ef.getChatChannel() == "" {
 		return ""

src/keybaseca/sshutils/generate_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// Test generating a new SSH key
 func TestGenerateNewSSHKey(t *testing.T) {
 	filename := "/tmp/bot-ssh-ca-integration-test-generate-key"
 	os.Remove(filename)

src/keybaseca/sshutils/generate_unix.go

@@ -8,8 +8,7 @@ import (
 	"strings"
 )
 
-// Generate a new SSH key. Places the private key at filename and the public key at filename.pub. If `overwrite`,
-// it will overwrite the existing key. If `printPubKey` it will print out the generated public key to stdout.
+// Generate a new SSH key. Places the private key at filename and the public key at filename.pub.
 // On unix, we use ed25519 keys since they may be more secure (and are smaller). The go crypto ssh library
 // does not support ed25519 keys so we use ssh-keygen in order to generate the key.
 func generateNewSSHKey(filename string) error {

src/keybaseca/sshutils/generate_windows.go

@@ -15,8 +15,7 @@ import (
 	"golang.org/x/crypto/ssh"
 )
 
-// Generate a new SSH key. Places the private key at filename and the public key at filename.pub. If `overwrite`,
-// it will overwrite the existing key. If `printPubKey` it will print out the generated public key to stdout.
+// Generate a new SSH key. Places the private key at filename and the public key at filename.pub.
 // On windows, we use 2048 bit rsa keys. go's ssh library doesn't support ed25519 and ssh-keygen isn't built in.
 func generateNewSSHKey(filename string) error {
 	privateKey, err := rsa.GenerateKey(rand.Reader, 2048)

src/keybaseca/sshutils/sshutils.go

@@ -17,6 +17,8 @@ import (
 	"github.com/google/uuid"
 )
 
+// Generate a new ssh key. Store the private key at filename and the public key at filename.pub. If overwrite, it will
+// overwrite anything at filename or filename.pub. If printPubKey, it will print the generated public key to stdout.
 func GenerateNewSSHKey(filename string, overwrite bool, printPubKey bool) error {
 	_, err := os.Stat(filename)
 	if err == nil {
@@ -46,8 +48,10 @@ func GenerateNewSSHKey(filename string, overwrite bool, printPubKey bool) error
 	return nil
 }
 
-func Generate(conf config.Config, overwrite bool, printPubKey bool) error {
-	err := GenerateNewSSHKey(conf.GetCAKeyLocation(), overwrite, printPubKey)
+// Generate a new CA key based off of the data in the config. If overwrite, it will overwrite the current CA key. Prints
+// the generated public key to stdout.
+func Generate(conf config.Config, overwrite bool) error {
+	err := GenerateNewSSHKey(conf.GetCAKeyLocation(), overwrite, true)
 	if err == nil {
 		log.Log(conf, fmt.Sprintf("Wrote new SSH CA key to %s", conf.GetCAKeyLocation()))
 	}
@@ -69,6 +73,8 @@ func getTempFilename(pattern string) (string, error) {
 	return tempFilename, nil
 }
 
+// Process a given SignatureRequest into a SignatureResponse or an error. This consists of validating the signature request,
+// determining the correct principals, and signing the provided public key.
 func ProcessSignatureRequest(conf config.Config, sr shared.SignatureRequest) (resp shared.SignatureResponse, err error) {
 	randomUUID, err := uuid.NewRandom()
 	if err != nil {

src/kssh/bot.go

@@ -103,6 +103,8 @@ func GetSignedKey(config ConfigFile, request shared.SignatureRequest) (shared.Si
 	}
 }
 
+// Get the configured channel name from the given config file. Returns either a pointer to the channel name string
+// or a null pointer.
 func getChannel(config ConfigFile) *string {
 	var channel *string
 	if config.ChannelName != "" {

src/kssh/config.go

@@ -11,15 +11,17 @@ import (
 	"github.com/keybase/bot-ssh-ca/src/shared"
 )
 
-// A ConfigFile that is provided by the keybaseca server process and lives in kbfs
+// A ConfigFile that is provided by the keybaseca server process and lives in kbfs. It is used to share configuration
+// information about how kssh should communicate with the keybaseca chatbot.
 type ConfigFile struct {
 	TeamName    string `json:"teamname"`
 	ChannelName string `json:"channelname"`
 	BotName     string `json:"botname"`
 }
 
 // LoadConfigs loads client configs from KBFS. Returns a (listOfConfigFiles, listOfBotNames, err)
-// Both lists are deduplicated based on ConfigFile.BotName
+// Both lists are deduplicated based on ConfigFile.BotName. Runs the KBFS operations in parallel
+// to speed up loading configs.
 func LoadConfigs() ([]ConfigFile, []string, error) {
 	allTeamsFromKBFS, err := shared.KBFSList("/keybase/team/")
 	if err != nil {
@@ -81,6 +83,7 @@ func LoadConfigs() ([]ConfigFile, []string, error) {
 	return configs, botnames, nil
 }
 
+// Load a kssh client config file from the given filename
 func LoadConfig(kbfsFilename string) (ConfigFile, error) {
 	var cf ConfigFile
 	if !strings.HasPrefix(kbfsFilename, "/keybase/") {
@@ -100,21 +103,26 @@ func LoadConfig(kbfsFilename string) (ConfigFile, error) {
 	return cf, err
 }
 
-// A LocalConfigFile is a file that lives on the FS of the computer running kssh. It is only used if the user is
-// in multiple teams that are running the CA bot. Note that we store the team in here (even though it wasn't specified
-// by the user) so that we can avoid doing a call to `LoadConfigs` if a default is set (since `LoadConfigs can be very
-// slow if the user is in a large number of teams).
+// A LocalConfigFile is a file that lives on the FS of the computer running kssh. By default (and for most users), this
+// file is not used.
 //
-// Controlled via `kssh --set-default-bot foo`.
+// If a user of kssh is in in multiple teams that are running the CA bot they can configure a default bot to communicate
+// with. Note that we store the team in here (even though it wasn't specified by the user) so that we can avoid doing
+// a call to `LoadConfigs` if a default is set (since `LoadConfigs can be very slow if the user is in a large number of teams).
+// This is controlled via `kssh --set-default-bot foo`.
+//
+// If a user of kssh wishes to configure a default ssh user to use (see README.md for a description of why this may
+// be useful) this is also stored in the local config file. This is controlled via `kssh --set-default-user foo`.
 type LocalConfigFile struct {
 	DefaultBotName string `json:"default_bot"`
 	DefaultBotTeam string `json:"default_team"`
 	DefaultSSHUser string `json:"default_ssh_user"`
 }
 
-// Where to store the local config file. Just stash it in ~/.ssh
+// Where to store the local config file. Just stash it in ~/.ssh rather than making a ~/.kssh folder
 var localConfigFileLocation = shared.ExpandPathWithTilde("~/.ssh/kssh-config.json")
 
+// Get the default SSH user to use for kssh connections. Empty if no user is configured.
 func GetDefaultSSHUser() (string, error) {
 	lcf, err := getCurrentConfig()
 	if err != nil {
@@ -124,6 +132,7 @@ func GetDefaultSSHUser() (string, error) {
 	return lcf.DefaultSSHUser, nil
 }
 
+// Set the default SSH user to use for kssh connections.
 func SetDefaultSSHUser(username string) error {
 	if strings.ContainsAny(username, " \t\n\r'\"") {
 		return fmt.Errorf("invalid username: %s", username)
@@ -138,6 +147,7 @@ func SetDefaultSSHUser(username string) error {
 	return writeConfig(lcf)
 }
 
+// Write the given config file to disk
 func writeConfig(lcf LocalConfigFile) error {
 	bytes, err := json.Marshal(&lcf)
 	if err != nil {
@@ -157,6 +167,7 @@ func writeConfig(lcf LocalConfigFile) error {
 	return nil
 }
 
+// Get the current kssh config file
 func getCurrentConfig() (lcf LocalConfigFile, err error) {
 	if _, err := os.Stat(localConfigFileLocation); os.IsNotExist(err) {
 		return lcf, nil
@@ -172,10 +183,12 @@ func getCurrentConfig() (lcf LocalConfigFile, err error) {
 	return lcf, nil
 }
 
+// Set the default keybaseca bot to communicate with.
 func SetDefaultBot(botname string) error {
 	teamname := ""
 	var err error
 	if botname != "" {
+		// Get the team associated with it and cache that too in order to avoid looking it up everytime
 		teamname, err = GetTeamFromBot(botname)
 		if err != nil {
 			return err
@@ -192,6 +205,7 @@ func SetDefaultBot(botname string) error {
 	return writeConfig(lcf)
 }
 
+// Get the default bot and team for kssh
 func GetDefaultBotAndTeam() (string, string, error) {
 	lcf, err := getCurrentConfig()
 	if err != nil {
@@ -200,6 +214,7 @@ func GetDefaultBotAndTeam() (string, string, error) {
 	return lcf.DefaultBotName, lcf.DefaultBotTeam, nil
 }
 
+// Get the teamname associated with the given botname
 func GetTeamFromBot(botname string) (string, error) {
 	configs, _, err := LoadConfigs()
 	if err != nil {

src/kssh/ssh.go

@@ -9,6 +9,7 @@ import (
 	"github.com/keybase/bot-ssh-ca/src/shared"
 )
 
+// Add the SSH key at the given location to the currently running SSH agent. Errors if there is no running ssh-agent.
 func AddKeyToSSHAgent(keyPath string) error {
 	cmd := exec.Command("ssh-add", keyPath)
 	bytes, err := cmd.CombinedOutput()
@@ -61,6 +62,7 @@ func CreateDefaultUserConfigFile() error {
 	return nil
 }
 
+// Make the ~/.ssh/ folder if it does not exist
 func MakeDotSSH() error {
 	if _, err := os.Stat(shared.ExpandPathWithTilde("~/.ssh/")); os.IsNotExist(err) {
 		err = os.Mkdir(shared.ExpandPathWithTilde("~/.ssh/"), 0700)

src/shared/chat_types.go

@@ -1,5 +1,14 @@
 package shared
 
+/*
+chat_types.go includes the types used when kssh and keybaseca communicate over keybase chat. kssh starts by sending a
+series of AckRequests in order to determine whether keybaseca is currently active and responding to messages. Keybaseca
+responds to AckRequests with an AckResponse. Both messages contain the username of the user using kssh in order to
+ensure that kssh is reading AckResponses that are meant for it (as opposed to another user of kssh). Then kssh sends
+a SignatureRequest. This is a json object prefix with a specific string. The json object contains the ssh public key
+and a uuid that is used to track the request. keybaseca responds with a signature response that contains the same uuid.
+*/
+
 import (
 	"encoding/json"
 	"fmt"
@@ -17,6 +26,7 @@ type SignatureRequest struct {
 // The preamble used at the start of signature request messages
 const SignatureRequestPreamble = "Signature_Request:"
 
+// Parse the given string as a serialized SignatureRequest
 func ParseSignatureRequest(body string) (SignatureRequest, error) {
 	if !strings.HasPrefix(body, SignatureRequestPreamble) {
 		return SignatureRequest{}, fmt.Errorf("ParseSignatureRequest called on a body without a preamble")
@@ -37,6 +47,7 @@ type SignatureResponse struct {
 // The preamble used at the start of signature response messages
 const SignatureResponsePreamble = "Signature_Response:"
 
+// Parse the given string as a serialized SignatureResponse
 func ParseSignatureResponse(body string) (SignatureResponse, error) {
 	if !strings.HasPrefix(body, SignatureResponsePreamble) {
 		return SignatureResponse{}, fmt.Errorf("ParseSignatureResponse called on a body without a preamble")
@@ -48,15 +59,22 @@ func ParseSignatureResponse(body string) (SignatureResponse, error) {
 	return sr, err
 }
 
+// Generate an AckRequest for the given username
 func GenerateAckRequest(username string) string {
 	return "AckRequest--" + username
 }
+
+// Generate an AckResponse in response to the given ack request
 func GenerateAckResponse(ackRequest string) string {
 	return strings.Replace(ackRequest, "AckRequest", "Ack", 1)
 }
+
+// Returns whether the given message is an ack request
 func IsAckRequest(msg string) bool {
 	return strings.HasPrefix(msg, "AckRequest--")
 }
+
+// Returns whether the given message is an ack response
 func IsAckResponse(msg string) bool {
 	return strings.HasPrefix(msg, "Ack--")
 }