-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Stash/Bitbucket Server initial support #1581
Conversation
func Load(env envconfig.Env) *BitbucketServer{ | ||
|
||
//Read | ||
config := env.String("REMOTE_CONFIG", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding it as separate env variables I would add it as part of the url and drop it from there while parsing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am talking about the separate username and password
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I'll make the change.
@josmo really good start! I know you are new to Go so I've provided some minor style and coding suggestions above. You will also need to resync with master since the code has merge conflicts. |
bitbucketserver := BitbucketServer{} | ||
bitbucketserver.URL = url_.String() | ||
bitbucketserver.GitUserName = gitUserName | ||
bitbucketserver.GitPassword = gitUserPassword |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as @tboerger mentioned above we should pass all parameters through the configuration string like this:
- bitbucketserver.GitUserName = gitUserName
- bitbucketserver.GitPassword = gitUserPassword
+ bitbucketserver.GitUserName = params.Get("git_username")
+ bitbucketserver.GitPassword = params.Get("git_password")
Moving git username and password to the url Removing un-needed setting of Allows Moving log.Fatal to log.Error Removing panics Moving to https for gravatar # Conflicts: # remote/remote.go
…till it's configurable
@bradrydzewski I think I have all the comments in and merged in master. The only thing is on how to handle the private key. For right now I put it in /var/lib/bitbucketserver/private_key.pem - it's about 50ish lines so if you think there's a better way to handle it I'll look into it. |
@josmo I recommend making the path configurable as part of the |
@@ -12,6 +12,7 @@ deps: | |||
go get -u github.com/elazarl/go-bindata-assetfs/... | |||
go get -u github.com/dchest/jsmin | |||
go get -u github.com/franela/goblin | |||
go get -u github.com/mrjones/oauth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also can you remove this line?
we vendor our dependencies using the govendor
tool. You can download the govendor tool with the following command:
go get -u github.com/kardianos/govendor
and then you can add the oauth dependency with the following line:
govendor add github.com/mrjones/oauth
note that if this is vendored properly you should only see mrjones/oauth added to the vendor directory and nothing else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool made the change. Had no idea about it
need go-fmt |
@@ -111,7 +105,7 @@ func (bs *BitbucketServer) Login(res http.ResponseWriter, req *http.Request) (*m | |||
bits, err := ioutil.ReadAll(response.Body) | |||
userName := string(bits) | |||
|
|||
response1, err := client.Get(bs.URL + "/rest/api/1.0/users/" +userName) | |||
response1, err := client.Get(bs.URL + "/rest/api/1.0/users/" + userName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt.Sprintf("%s/rest/api/1.0/users/%s", bs.URL, userName)
for string interpolation
I've corrected this line in PR #1589 |
Got the same error when tested against
it may be creadential issue, but no readable error showed |
@valichek I'll have to add a bit insightful errors in it. From where that errors takes places it's probably due to the private key. Did you generate a private/public key, something like openssl genrsa -out private_key.pem 4096 and then made sure the private key was in the location for ${consumer_rsa} that the container has access to? |
@josmo I managed to authenticate stash, but it was really tricky, would be nice to have some pics in docs because the workflow differs much from github auth |
Hi, I would like to report that it was working fine for authentication and more up to activation of a repo. Activation failed due to a typo which is why I submitted my PR. The commits after my PR seem to have broken something though... now I can't even start drone due to some sort of failure related to the stash/bitbucketserver plugin. I was using the exact same config as when it worked. I just pulled the latest docker image. |
@mvdkleijn I had to add |
@josmo I have authorized drone for stash, I can browse repos, I have activated one, but when I push new commit, drone does nothing, no build started, no errors in log, do you have an idea what is going on? How does drone handles pushes to stash? And sure, I have added .drone.yml |
@valichek The first thing I would check is under that repos settings in stash under hooks. There should be a web-hook plugin thats enabled and has the url to call drone on pushes. What version of stash are you on? I've only tested on 4.5 |
@josmo The only webhook I have is for Jenkins, could you show how it should look like? Screenshot? Stash has 4.4.1 version |
@valichek That last one is the webhook post I believe is included with Stash (at least it's included in all the instances I'm working with ). It should work on 4.4.1 |
@josmo and what is an url for webhook? |
it should be something like http:https://{baseurl to your drone instance}/hook?access_token={the access token drone gave stash when setting up the hook} |
@josmo So this webhook should be automatically created when creating app link? |
yep yep as long as the account you are using to link has permissions on that repository |
@josmo is that means I need admin account for stash to activate repo |
no just the repo not all of stash |
Thanks @valicheck, I'll try that |
There is a bug here https://github.com/drone/drone/blob/master/remote/bitbucketserver/bitbucketserver.go#L216
|
@josmo - The post receive webhook you're referring to is a plugin that's not installed by default. It is created by Atlassian Labs (and therefore unsupported). You can find it here: https://marketplace.atlassian.com/plugins/com.atlassian.stash.plugin.stash-web-post-receive-hooks-plugin/versions#b312 |
Adding |
@mvdkleijn Oh good to know. It's been included on each bitbucket server I've used. I'll add that to the docs when I get a chance (unless you want to add a create a PR :)) @valichek Oh yeah I think I remember thinking that should be in there and forgot about it. I'll have some time later this week to fix this unless you want to do it :) |
@josmo I just wanted to follow up and see if you are planning on a second set of pull requests for Bitbucket Server? I do have some concerns with the initial implementation, such as all repositories being marked public, all users being granted admin access, and skipping ssl verification by default. Since these are security related we'll need to get them resolved or else I'll be forced to disable the implementation (not trying to be a jerk but I don't want someone getting hacked and blaming drone) |
@bradrydzewski Sorry, i've gotten caught on up a few things and haven't had a chance to work on this for a few. I'm definitely planning on adding a new set of changes to add perms. I just haven't had time to dig down into how the perms are used but I'll try to get to it soon :) |
@bradrydzewski I was starting look at the permissions and address the comments but it looks like it's a bit in the middle of .5/.4 release status. Should I wait after .5 is out? I don't want to step on anything and make sure everything builds :) |
@josmo I'm having difficulty getting this remote driver to work using BitbucketServer 4.6.1 and Drone 0.4.2. I manually generated my public/private keypair and setup the application link in BitbucketServer, then configured my dronerc with the appropriate consumer_key and path to drone.pem. I navigate to $dronehost/login, hit login and get redirected to Stash where I successfully allow drone access to stash. I'm then sent to a confirmation page saying "Access Approved" with a verification code. I don't see anything in the drone logs after the redirect to Bitbucket, and don't see drone in my users application links. I'm wondering if I need more than just the application authorize url in my Bitbucket application link configuration? |
@dreadpirate15 I haven't looked at Bitbucket 4.6 yet, although I don't think anything would have changed to break it. Did you set up the redirect back to drone in the application link under incoming authentication (http:https://{drone host name}/authorize/)? You shouldn't see the verfication code, that should be sent back to drone on the authorize. Normally you should see drone in your user authorized applications. |
What is the status of this integration's security improvement, @josmo ? |
This is a list of things I'd like to see improved for the Stash implementation. Note that @josmo could probably use some help. I know there has been a lot of interest in this feature, so I would welcome others to contribute to this implementation. API the bitbucket API calls should be encapsulated in a library similar to bitbucket cloud in an Security the bitbucket api should be used to fetch permissions instead of granting all users default, admin access to repositories. See https://github.com/drone/drone/blob/ebd547deacce9de774d609261d4049c99883945c/remote/bitbucketserver/bitbucketserver.go#L206 Testing see the bitbucket cloud package for reference. This package has near 100% coverage Style the overall code style is very different from the other remote implementations. The stash implementation should try to closely resemble these other packages. I recommend looking at the bitbucket cloud package and trying to emulate the same style and structure as much as possible. |
@gdubicki I was in process of working on it for .5 when I got side tracked to fix the clone issue on it. I'll take a look at the security next. Then I"ll see about matching the API and style to bitbucket cloud (with testing). For security I imagine I'll have to do the same hack as bitbucket cloud since there isn't an api but will have to look at the hook permissions to it. @bradrydzewski For the Security to be considered ok. If I follow the same suite a bitbucket cloud does that seem ok to you? I definitely wouldn't mind having some help on it since I'm pretty new at this. Thanks @bradrydzewski |
This is the first go to get Stash/Bitbucket support working(continuation of #567). The "happy path" works and there's a bit of clean up (First time I've written anything in golang) but if folks want to try it out or give some feedback on the dos and don'ts I'll update it.